ENS contest - durianSausage's results

Lines of code

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

Vulnerability details

problem

We should use address(xxx).call{value:xxxx} instead of payable(msg.sender).transfer . Because if msg.sender is a contract, it can use its receive function to reentrance attacking or using some logic vulner abilities to lock the entire contract.

prof:

ETHRegisterController.sol, 182

        if (msg.value > (price.base + price.premium)) {
            payable(msg.sender).transfer(
                msg.value - (price.base + price.premium)
            );
        }

we should use the following codes.

        if (msg.value > (price.base + price.premium)) {
            ret = payable(msg.sender).call{value:msg.value - (price.base + price.premium)}();
			if ret xxxx
        }

---

ETHRegisterController.sol, 204

        if (msg.value > price.base) {
            payable(msg.sender).transfer(msg.value - price.base);
        }

we should use the following codes.

        if (msg.value > price.base) {
			ret = payable(msg.sender).call{value:msg.value - price.base}();
     		if ret xxxx
        }

---

ETHRegisterController.sol, 210

    function withdraw() public {
        payable(owner()).transfer(address(this).balance);
    }

we should use the following codes.

    function withdraw() public {
        ret = payable(msg.sender).call{value: address(this).balance)}();
        if ret xxxx
    }

tools

vim

gas optimization

G01: COMPARISONS WITH ZERO FOR UNSIGNED INTEGERS

problem

0 is less gas efficient than !0 if you enable the optimizer at 10k AND you’re in a require statement. Detailed explanation with the opcodes https://twitter.com/gzeon/status/1485428085885640706

prof

'BytesUtil.sol', 43, ' if(len > 0) { 'ETHRegistrarController.sol', 97, ' if (data.length > 0) { 'RRUtils.sol', 244, ' while (counts > 0 && !self.equals(off, other, otheroff)) {

G02: custom error save more gas

problem

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained https://blog.soliditylang.org/2021/04/21/custom-errors/. Custom errors are defined using the error statement.

prof

'BytesUtil.sol', 12, ' require(offset + len <= self.length); 'BytesUtil.sol', 27, ' require(offset == self.length - 1, "namehash: Junk at end of name"); 'BytesUtil.sol', 41, ' require(idx < self.length, "readLabel: Index out of bounds"); 'BytesUtils.sol', 11, ' require(offset + len <= self.length); 'BytesUtils.sol', 145, ' require(idx + 2 <= self.length); 'BytesUtils.sol', 158, ' require(idx + 4 <= self.length); 'BytesUtils.sol', 171, ' require(idx + 32 <= self.length); 'BytesUtils.sol', 184, ' require(idx + 20 <= self.length); 'BytesUtils.sol', 198, ' require(len <= 32); 'BytesUtils.sol', 199, ' require(idx + len <= self.length); 'BytesUtils.sol', 234, ' require(offset + len <= self.length); 'BytesUtils.sol', 261, ' require(len <= 52); 'BytesUtils.sol', 267, ' require(char >= 0x30 && char <= 0x7A); 'BytesUtils.sol', 269, ' require(decoded <= 0x20); 'Controllable.sol', 16, ' require(controllers[msg.sender], "Controllable: Caller is not a controller"); 'ERC1155Fuse.sol', 59, ' require( 'ERC1155Fuse.sol', 84, ' require( 'ERC1155Fuse.sol', 106, ' require( 'ERC1155Fuse.sol', 175, ' require(to != address(0), "ERC1155: transfer to the zero address"); 'ERC1155Fuse.sol', 176, ' require( 'ERC1155Fuse.sol', 194, ' require( 'ERC1155Fuse.sol', 198, ' require(to != address(0), "ERC1155: transfer to the zero address"); 'ERC1155Fuse.sol', 199, ' require( 'ERC1155Fuse.sol', 214, ' require( 'ERC1155Fuse.sol', 247, ' require(owner == address(0), "ERC1155: mint of existing token"); 'ERC1155Fuse.sol', 248, ' require(newOwner != address(0), "ERC1155: mint to the zero address"); 'ERC1155Fuse.sol', 249, ' require( 'ERC1155Fuse.sol', 289, ' require( 'ETHRegistrarController.sol', 56, ' require(_maxCommitmentAge > _minCommitmentAge); 'ETHRegistrarController.sol', 98, ' require( 'ETHRegistrarController.sol', 100, ' "ETHRegistrarController: resolver is required when data is supplied" 'ETHRegistrarController.sol', 120, ' require(commitments[commitment] + maxCommitmentAge < block.timestamp); 'ETHRegistrarController.sol', 136, ' require( 'ETHRegistrarController.sol', 195, ' require( 'ETHRegistrarController.sol', 231, ' require( 'ETHRegistrarController.sol', 237, ' require( 'ETHRegistrarController.sol', 241, ' require(available(name), "ETHRegistrarController: Name is unavailable"); 'ETHRegistrarController.sol', 245, ' require(duration >= MIN_REGISTRATION_DURATION); 'ETHRegistrarController.sol', 258, ' require( 'Owned.sol', 9, ' require(msg.sender == owner); 'ReverseRegistrar.sol', 40, ' require( 'ReverseRegistrar.sol', 51, ' require( 'RRUtils.sol', 306, ' require(data.length <= 8192, "Long keys not permitted");

G03: ARRAY LENTH SHOULD USE MEMoRY VARIABLE LOAD IT

problem

The overheads outlined below are PER LOOP, excluding the first loop

storage arrays incur a Gwarmaccess (100 gas) memory arrays use MLOAD (3 gas) calldata arrays use CALLDATALOAD (3 gas) Caching the length changes each of these to a DUP (3 gas), and gets rid of the extra DUP needed to store the stack offset

prof

'DNSSECImpl.sol', 92, ' for(uint i = 0; i < input.length; i++) { 'ERC1155Fuse.sol', 91, ' for (uint256 i = 0; i < accounts.length; ++i) { 'ERC1155Fuse.sol', 204, ' for (uint256 i = 0; i < ids.length; ++i) { 'ETHRegistrarController.sol', 255, ' for (uint256 i = 0; i < data.length; i++) { 'RRUtils.sol', 285, ' * for (uint i = 0; i < data.length; i++) { 'RRUtils.sol', 309, ' for(uint i = 0; i < data.length + 31; i += 32) { 'RRUtils.sol', 314, ' if(i + 32 > data.length) { 'RRUtils.sol', 315, ' uint unused = 256 - (data.length - i) * 8;

G04: the assert() function when false, uses up all the remaining gas and reverts all the changes made.

problem

assert(false) compiles to 0xfe, which is an invalid opcode, using up all remaining gas, and reverting all changes. require(false) compiles to 0xfd which is the REVERT opcode, meaning it will refund the remaining gas. The opcode can also return a value (useful for debugging), but I don't believe that is supported in Solidity as of this moment. (2017-11-21)

prof

'RRUtils.sol', 21, ' assert(idx < self.length); 'RRUtils.sol', 51, ' assert(offset < self.length);

G05: PREFIX INCREMENT SAVE MORE GAS

problem

prefix increment ++i is more cheaper than postfix i++

prof

'BytesUtils.sol', 265, ' for(uint i = 0; i < len; i++) { 'BytesUtils.sol', 312, ' for(uint256 idx = off; idx < off + len; idx++) { 'DNSSECImpl.sol', 92, ' for(uint i = 0; i < input.length; i++) { 'ETHRegistrarController.sol', 255, ' for (uint256 i = 0; i < data.length; i++) { 'RRUtils.sol', 285, ' * for (uint i = 0; i < data.length; i++) { 'StringUtils.sol', 13, ' for(len = 0; i < bytelength; len++) {

G06: > and < comparing with constant are more efficient than >= and <=

problem

If we use > and < comparing with constant in statement than >= and <=, we will save more gas. Because we can use less evm bytecode to run our codes.

prof

'BytesUtils.sol', 198, ' require(len <= 32); 'BytesUtils.sol', 261, ' require(len <= 52); 'BytesUtils.sol', 267, ' require(char >= 0x30 && char <= 0x7A); 'BytesUtils.sol', 269, ' require(decoded <= 0x20); 'RRUtils.sol', 306, ' require(data.length <= 8192, "Long keys not permitted");

G07: use type(uint64).max storage variable will cost more gas than embeded bytecode or dynamic generated value.

problem

storage variable will cost more gas when we use SREAD. So we should not use the storage varible to store the varibles like type(uint64).max.

prof

NameWrapper.sol, 47, uint64 private constant MAX_EXPIRY = type(uint64).max;

        _setData(
            uint256(ETH_NODE),
            address(0),
            uint32(PARENT_CANNOT_CONTROL | CANNOT_UNWRAP),
            MAX_EXPIRY
        );
        _setData(
            uint256(ROOT_NODE),
            address(0),
            uint32(PARENT_CANNOT_CONTROL | CANNOT_UNWRAP),
            MAX_EXPIRY
        );

    function uint256max() public pure returns(uint256) {
        return type(uint64).max;
    }
	
	
	
	     _setData(
            uint256(ETH_NODE),
            address(0),
            uint32(PARENT_CANNOT_CONTROL | CANNOT_UNWRAP),
            uint256max()
        );
        _setData(
            uint256(ROOT_NODE),
            address(0),
            uint32(PARENT_CANNOT_CONTROL | CANNOT_UNWRAP),
            uint256max()
        );