ENS contest - zzzitron's results

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L657-L674 https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/ERC1155Fuse.sol#L274-L284

Vulnerability details

Impact

HIGH - bypassing PARENT_CANNOT_CONTROL fuse As discussed in the discord, bypassing fuse is considered high, thus it is reported as high impact

Conditions for the parent for this exploit:

• should be able to unwrap: no CANNOT_UNWRAP fuse on the parent node
• should be able to create subnode: no CANNOT_CREATE_SUBDOMAIN fuse on the parent node

Or alternatively, the parent node is not wrapped in the first place

Proof of Concept

• proof of concept: "poc parent can bypass PARENT_CANNOT_CONTROL"

The proof of concept demonstrates how to bypass PARENT_CANNOT_CONTROL fuse and set fuses for child's node.

1. unwrap the parent's node
2. call on ENSRegistry::setSubnodeOwner for the child node and set the owner to zero address
3. wrap the parent's node again
4. call on NameWrapper::setSubnodeOwner or NameWRapper::setSubnodeRecord to change the fuse to zero

This is similar to the issue NameWrapper: parent can change owner of subnode by minting, but this proof of concept combines other issues for an alternative path to exploit. This exploit will work even when the mint of burn functions are checking the existing fuses.

By setting the childnode's owner in the ENS Registry, one can bypass the PARENT_CANNOT_CONTROL fuse:

// https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L657-L674
// wrapper/NameWrapper.sol::canCallSetSubnodeOwner
// both setSubnodeOwner and setSubnodeRecord are using this modifier
// setting the owner in the ENS registry to zero will bypass `PARENT_CANNOT_CONTROL` fuse

657     modifier canCallSetSubnodeOwner(bytes32 node, bytes32 labelhash) {
658         bytes32 subnode = _makeNode(node, labelhash);
659         address owner = ens.owner(subnode);
660
661         if (owner == address(0)) {
662             (, uint32 fuses, ) = getData(uint256(node));
663             if (fuses & CANNOT_CREATE_SUBDOMAIN != 0) {
664                 revert OperationProhibited(node);
665             }
666         } else {
667             (, uint32 subnodeFuses, ) = getData(uint256(subnode));
668             if (subnodeFuses & PARENT_CANNOT_CONTROL != 0) {
669                 revert OperationProhibited(node);
670             }
671         }
672
673         _;
674     }

A bug in the ERC1155Fuse::_transfer function was used. It will not revert even when the sender does not own the token, if it is sent to the current owner. In the proof of concept, the parent does not own the ERC-1155 wrapper token of the child node, but the transfer function did not revert, since the token was sent to the current owner.

// https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/ERC1155Fuse.sol#L274-L284
// wrapper/ERC1155Fuse.sol::_transfer
// line 283 does not revert even when the sender does not own the token
// if it is sent to the oldOwner

274     function _transfer(
275         address from,
276         address to,
277         uint256 id,
278         uint256 amount,
279         bytes memory data
280     ) internal {
281         (address oldOwner, uint32 fuses, uint64 expiry) = getData(id);
282         if (oldOwner == to) {
283             return;
284         }

Tools Used

harahat

Recommended Mitigation Steps

The NameWrapper assumes when the address zero is the owner, the token was not minted. But in the ENS Registry one can check the owner to zero.

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

L00: Usage of transfer to send eth

The transfer function has a fixed gas stipend of 2300. If a contract as well as EOA can call the function, it is advised to use call function instead of transfer.

Here are transfer function used in the ETHRegistrarController.sol:

• https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L183-L185
• https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L203-L205
• https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L183-L185
• https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L210-L212

Lines of code

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

Vulnerability details

Impact

MED - the function of the protocol could be impacted

compare will return false answer without reverting when the inputs are not valid.

Proof of Concept

The compare function is used for compareNames. The names are supposed to be DNS wire format. If the strings are malformed, it is possible to give out-of-range offset, len, otheroffset, and otherlen. When it happens, the compare will return some false values, without reverting, since the validity of offset and len are not checked.

// https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/BytesUtils.sol#L44-L51
// dnssec-oracle/BytesUtils.sol::compare
// The length of self and other are not enforced

 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;

Tools Used

none

Recommended Mitigation Steps

Check whether the offset, len are within the length of self, as well as for the other.

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/RRUtils.sol#L266-L268 https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/DNSSECImpl.sol#L126-L134

Vulnerability details

Impact

MED - the function of the protocol could be impacted

The serialNumberGte will revert when one of the serial number is greater than the max value of int32.

Proof of Concept

// https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/DNSSECImpl.sol#L126-L134
// dnssec-oracle/DNSSECImpl.sol::validateSignedSet

126         if(!RRUtils.serialNumberGte(rrset.expiration, uint32(now))) {
127             revert SignatureExpired(rrset.expiration, uint32(now));
128         }

132         if(!RRUtils.serialNumberGte(uint32(now), rrset.inception)) {
133             revert SignatureNotValidYet(rrset.inception, uint32(now));
134         }

The validateSignedSet is using serialNumberGte with timestamp values.

// https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/dnssec-oracle/RRUtils.sol#L266-L268
// dnssec-oracle/RRUtils.sol::serialNumberGte
// serialNumberGte is used in DNSSECImple::validateSignedSet
// the i1 and i2 are timestamps

266     function serialNumberGte(uint32 i1, uint32 i2) internal pure returns(bool) {
267         return int32(i1) - int32(i2) >= 0;
268     }

If one of the parameters of serialNumberGte is greater than the max of the int32 while the other parameter is not, the difference between them are bigger than int32 and it causes overflow or underflow revert. For example, RRUtils.serialNumberGte(2147483648, 1658130036) will revert with (Arithmetic operation underflowed or overflowed outside of an unchecked block). The first parameter 2147483648 is about 16 years from now (Jan/19/2038), and the second parameter 1658130036 is Jul/18/2022. The timestamp can be an expiration, so if the expiration is set to be greater than roughly 2038, validateSignedSet will always revert.

Tools Used

none

Recommended Mitigation Steps

Use unchecked block

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/ERC1155Fuse.sol#L274-L284

Vulnerability details

Impact

MED - the function of the protocol could be impacted

The safeTransferFrom does not comply with the ERC1155 standard when the token is sent to the old owner.

Proof of Concept

According to the EIP-1155 standard for the safeTransferFrom:

MUST revert if balance of holder for token _id is lower than the _value sent.

Let's say alice does not hold any token of tokenId, and bob holds one token of tokenId. Then alice tries to send one token of tokenId to bob with safeTranferFrom(alice, bob, tokenId, 1, ""). In this case, even though alice's balance (= 0) is lower than the amount (= 1) sent, the safeTransferFrom will not revert. Thus, violating the EIP-1155 standard. It can cause problems for other contracts using this token, since they assume the token was transferred if the safeTransferFrom does not revert. However, in the example above, no token was actually transferred.

// https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/ERC1155Fuse.sol#L274-L284
// wrapper/ERC1155Fuse.sol::_transfer
// ERC1155Fuse::safeTransferFrom uses _transfer

274     function _transfer(
275         address from,
276         address to,
277         uint256 id,
278         uint256 amount,
279         bytes memory data
280     ) internal {
281         (address oldOwner, uint32 fuses, uint64 expiry) = getData(id);
282         if (oldOwner == to) {
283             return;
284         }

Tools Used

none

Recommended Mitigation Steps

Revert even if the to address already owns the token.

Summary

Low

L00: Usage of transfer to send eth

The transfer function has a fixed gas stipend of 2300. If a contract as well as EOA can call the function, it is advised to use call function instead of transfer.

Here are transfer function used in the ETHRegistrarController.sol:

• https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L183-L185
• https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L203-L205
• https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L183-L185
• https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/ethregistrar/ETHRegistrarController.sol#L210-L212

L01: Lack of zero address check

Functions without checks on the zero address may cause loss of token

• NameWrapper::onERC721Received
  • the newly minted ERC1155 wrap can be transferred to zero address
• NameWrapper::setRecord
  • the token can be transferred to zero address

L02: Mismatching NatSpec