ENS contest - alan724's results

Judge has assessed an item in Issue #238 as Medium risk. The relevant finding follows:

2. call() should be used instead of transfer() on an address payable

Sometimes this kind of issue is considered as Medium risk.

The use of the deprecated transfer() function for an address will inevitably make the transaction fail when:

• The claimer smart contract does not implement a payable function.
• The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit.
• The claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call's gas usage above 2300.

Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L182

File: contracts\ethregistrar\ETHRegistrarController.sol
182:         if (msg.value > (price.base + price.premium)) {
183:             payable(msg.sender).transfer(
184:                 msg.value - (price.base + price.premium)
185:             );
186:         }

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L203

File: contracts\ethregistrar\ETHRegistrarController.sol
203:         if (msg.value > price.base) {
204:             payable(msg.sender).transfer(msg.value - price.base);
205:         }

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L210

File: contracts\ethregistrar\ETHRegistrarController.sol
210:     function withdraw() public {
211:         payable(owner()).transfer(address(this).balance);
212:     }

1. Use safeTransferFrom instead of transferFrom for ERC721 transfers

OpenZeppelin’s documentation discourages the use of transferFrom(), use safeTransferFrom() whenever possible. https://github.com/code-423n4/2022-07-ens/blob/main/contracts/wrapper/NameWrapper.sol#L231

File: contracts\wrapper\NameWrapper.sol
229: 
230:         // transfer the token from the user to this contract
231:         registrar.transferFrom(registrant, address(this), tokenId);
232: 
233:         // transfer the ens record back to the new owner (this contract)
234:         registrar.reclaim(tokenId, address(this));
235: 

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

File: contracts\wrapper\NameWrapper.sol
335:     function unwrapETH2LD(
336:         bytes32 labelhash,
337:         address newRegistrant,
338:         address newController
339:     ) public override onlyTokenOwner(_makeNode(ETH_NODE, labelhash)) {
340:         _unwrap(_makeNode(ETH_NODE, labelhash), newController);
341:         registrar.transferFrom(
342:             address(this),
343:             newRegistrant,
344:             uint256(labelhash)
345:         );
346:     }

2. call() should be used instead of transfer() on an address payable

Sometimes this kind of issue is considered as Medium risk.

The use of the deprecated transfer() function for an address will inevitably make the transaction fail when:

• The claimer smart contract does not implement a payable function.
• The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit.
• The claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call's gas usage above 2300.

Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L182

File: contracts\ethregistrar\ETHRegistrarController.sol
182:         if (msg.value > (price.base + price.premium)) {
183:             payable(msg.sender).transfer(
184:                 msg.value - (price.base + price.premium)
185:             );
186:         }

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L203

File: contracts\ethregistrar\ETHRegistrarController.sol
203:         if (msg.value > price.base) {
204:             payable(msg.sender).transfer(msg.value - price.base);
205:         }

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L210

File: contracts\ethregistrar\ETHRegistrarController.sol
210:     function withdraw() public {
211:         payable(owner()).transfer(address(this).balance);
212:     }

3. Unbounded loops with external calls

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L167

File: contracts\ethregistrar\ETHRegistrarController.sol
125:     function register(
126:         string calldata name,
...
131:         bytes[] calldata data,
...
135:     ) public payable override {
...
166: 
167:         _setRecords(resolver, keccak256(bytes(name)), data);
...
249:     function _setRecords(
250:         address resolver,
251:         bytes32 label,
252:         bytes[] calldata data
253:     ) internal {
254:         // use hardcoded .eth namehash
255:         bytes32 nodehash = keccak256(abi.encodePacked(ETH_NODE, label));
256:         for (uint256 i = 0; i < data.length; i++) {
...  

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/BulkRenewal.sol#L56

File: contracts\ethregistrar\BulkRenewal.sol
50:     function renewAll(string[] calldata names, uint256 duration)
51:         external
52:         payable
53:         override
54:     {
55:         ETHRegistrarController controller = getController();
56:         for (uint256 i = 0; i < names.length; i++) {
...

4. Wrong Comment

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

File: contracts\wrapper\NameWrapper.sol
367:     /**
368:      * @notice Sets fuses of a name
369:      * @param node namehash of the name
370:      * @param fuses fuses to burn (cannot burn PARENT_CANOT_CONTROL)
371:      */
372: 
373:     function setFuses(bytes32 node, uint32 fuses)

PARENT_CANOT_CONTROL should be PARENT_CANNOT_CONTROL

5. Wrong comparison result when the length is longer than 32

File: contracts\dnssec-oracle\BytesUtils.sol
44:     function compare(bytes memory self, uint offset, uint len, bytes memory other, uint otheroffset, uint otherlen) internal pure returns (int) {
45:         uint shortest = len;
46:         if (otherlen < len)
47:         shortest = otherlen;
48: 
49:         uint selfptr;
50:         uint otherptr;
51: 
52:         assembly {
53:             selfptr := add(self, add(offset, 32))
54:             otherptr := add(other, add(otheroffset, 32))
55:         }
56:         for (uint idx = 0; idx < shortest; idx += 32) {
57:             uint a;
58:             uint b;
59:             assembly {
60:                 a := mload(selfptr)
61:                 b := mload(otherptr)
62:             }
63:             if (a != b) {
64:                 // Mask out irrelevant bytes and check again
65:                 uint mask;
66:                 if (shortest > 32) {
67:                     mask = type(uint256).max;
68:                 } else {
69:                     mask = ~(2 ** (8 * (32 - shortest + idx)) - 1);
70:                 }
71:                 int diff = int(a & mask) - int(b & mask);
72:                 if (diff != 0)
73:                 return diff;
74:             }
75:             selfptr += 32;
76:             otherptr += 32;
77:         }
78: 
79:         return int(len) - int(otherlen);
80:     }

The comparison will be wrong when then shortest > 32 because the mask is wrong. For example when the parameters are 01234567890123456789012345678901xaxa, 0, 35 01234567890123456789012345678901xaxb, 0, 35, the result should be zero because they are same with the first 35 characters. For the 2nd iteration of L56, the shortest is greater than 32, and the mask will be type(uint256).max and it will mask all values and this will result to diff != 0.

• Recommended Mitigration: compare shortest-idx to 32 at line L66

66:                 if (shortest - idx > 32) {
67:                     mask = type(uint256).max;

5. Wrong comparison result when the self is longer than other

https://github.com/code-423n4/2022-07-ens/blob/main/contracts/dnssec-oracle/BytesUtils.sol#L116

File: contracts\dnssec-oracle\BytesUtils.sol
115:     function equals(bytes memory self, uint offset, bytes memory other) internal pure returns (bool) {
116:         return self.length >= offset + other.length && equals(self, offset, other, 0, other.length);
117:     }

When self.length > offset + other.length, the result can be true. For example when the parameters are hello1, 1, ello, the result should be false because ello1 is different from ello. But the result will be true with this function because the equals function will compare the string within the len.