Insufficient Validation For ERC721 Receive Hook Based Name Wrapping

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L710

Vulnerability details

Impact

The NameWrapper contract allows the direct wrapping of ERC721 .eth name tokens by leveraging the ERC721 standard's onERC721Received hook. This is achieved by decoding certain parameters from the data parameter which are expected to be packed along with the safeTransferFrom call. Most critically the new owner of the wrapped name is determined based on the passed data.

As part of a phishing attack an attacker could simply change the owner in the data parameter. Even a careful user who checks the address of the contract they're interacting with, function selector, recipient address and token being sent could still successfully be tricked due to a small change in the unstructured data. Common web3 interfaces such as Metamask and hardware wallets such as Trezor give users the ability to more easily parse
and verify function parameters but not arbitrary byte parameters.

Arguably assuming a user has been tricked into going to a malicious UI is already a strong a assumption, this has nevertheless been submitted as a medium severity issue as the current contract design strongly weakens a verification tool common users should be able to utilize to verify the integrity transactions.

Proof of Concept

Exploit scenario:

1. Trick user into submitting a transaction with the owner of the wrapped token in data being set to an attacker controlled address
2. Unwrap the name from the NameWrapper contract

Tools Used

• Manual review

Recommended Mitigation Steps

Add more verification of the decoded data. This could be achieved by requiring the addition of an extra signature to the packed data. Specifically I'd recommend packing the added parameters into a structure according to EIP-712, have the user sign the struct and then pack the signature along with the parameters into the data parameter. Since many wallets already natively support EIP712, this will ensure that users will practically have the chance to properly validate all transaction parameters before submitting.

[jefflau]

Recommend reducing severity to 0.

If a user is willing to transfer their name to some smart contract address with opaque data, not including an owner address won't make any difference. If you can convince them to do that you can almost certainly convince them to just send it to you.

[dmvt]

Yeah I agree with the sponsor here. This is a decent recommendation, but it doesn't really solve the "user blindly trusting ENS" issue. Downgrading to QA.