Update to 'not-so-testy' NonMatchingSelector tests and fixed failing TSOwnable.t.sol file with update to foundry-huff

[AmadiMichael]

Great work here guys! just wanted to contribute to fixes to my findings

/// @notice Test that a non-matching selector reverts
  function testNonMatchingSelector(bytes32 callData) public {
    bytes8[] memory func_selectors = new bytes8[](2);
    func_selectors[0] = bytes8(hex"13af4035");
    func_selectors[1] = bytes8(hex"8da5cb5b");

    bytes8 func_selector = bytes8(callData >> 0xe0);
    for (uint256 i = 0; i < 2; i++) {
      if (func_selector != func_selectors[i]) {
        return;
      }
    }

    address target = address(owner);
    uint256 OneWord = 0x20;
    bool success = false;
    assembly {
      success := staticcall(
          gas(),
          target,
          add(callData, OneWord),
          mload(callData),
          0,
          0
      )
    }
    assert(!success);
  }

This test found in most auth tests is bound to always succeed without really testing it, actually there's no chance a call to this function would reach the assembly part.

Example: if the fuzzer parses in 0xb10e2d527612073b26eecdfd717e6a320cf44b4afac2b0732d9fcbe2b7fa0cf6, the array is filled up with the function sigs the contract has but then, firstly this line bytes8 func_selector = bytes8(callData >> 0xe0); is bound to return 0x0000000000000000. Using the bytes32 above this will firstly execute (0xb10e2d527612073b26eecdfd717e6a320cf44b4afac2b0732d9fcbe2b7fa0cf6 >> 0xe0) which is 0xb10e2d52 with 224 bits of zero to the left. Performing bytes8(0xb10e2d52) will only take the first 8 bytes of this which is all zeroes.

I fixed it in assembly with

bytes4 func_selector;
        assembly {
            func_selector := and(
                callData,
                0xffffffff00000000000000000000000000000000000000000000000000000000
            )
        }

Secondly, even if the above issue didn't exist, the if statement states that if (func_selector != func_selectors[i]) { return; } while looping through the array of signatures. The issue is that rather than checking that if the shifted bytes above returns the same bytes as a function sig in the array it should return as that's not the aim of the test but rather it returns if the shifted bytes is different from any one in the array meaning that it would only get to the assembly part if the sig is the same and which will obviously pass. The fix was to change != to ==

The third issue was the assembly part. It wasn't detected i believe because the test never got there but if it did the execution would have ran out of gas. In the call it asks the evm to read from add(callData, OneWord) which could be as much as uint256.max + 0x80 also it asks it to read up to the latter plus mload(calldata) which could be doubling the size minus 0x80.

The fix was this

bool success = false;
        assembly {
            mstore(0x80, callData)
            // if its a state changing call this will return 0
            success := staticcall(gas(), target, 0x80, 0x20, 0, 0)

            if iszero(success) {
                success := call(gas(), target, 0, 0x80, 0x20, 0, 0)
            }
        }

Checking a static call first incase which if it fails we make a call. mstoring calldata in memory and using it.

The last part involveed adding config2 and getConfig2Address to foundry-huff to work. TSOwnable failed because the owner couldn't be determined deterministically so create2 could have worked but then it wasn;t available in foundry-huff

To make this work just add

function config2(uint256 salt) public returns (HuffConfig) {
        return new HuffConfig{salt: bytes32(salt)}();
    }

    function getConfig2Address(uint256 salt) public view returns (address) {
        return
            address(
                uint160(
                    uint256(
                        keccak256(
                            abi.encodePacked(
                                bytes1(0xff),
                                address(this),
                                bytes32(salt),
                                keccak256(type(HuffConfig).creationCode)
                            )
                        )
                    )
                )
            );
    }

to foundry-huff in lib/foundry-huff/src/HuffDeployer.sol

A pull request with tests to this (basically dups of the create tests) is in foundry-huff too.

[AmadiMichael]

Great work here guys! just wanted to contribute to fixes to my findings

/// @notice Test that a non-matching selector reverts
  function testNonMatchingSelector(bytes32 callData) public {
    bytes8[] memory func_selectors = new bytes8[](2);
    func_selectors[0] = bytes8(hex"13af4035");
    func_selectors[1] = bytes8(hex"8da5cb5b");

    bytes8 func_selector = bytes8(callData >> 0xe0);
    for (uint256 i = 0; i < 2; i++) {
      if (func_selector != func_selectors[i]) {
        return;
      }
    }

    address target = address(owner);
    uint256 OneWord = 0x20;
    bool success = false;
    assembly {
      success := staticcall(
          gas(),
          target,
          add(callData, OneWord),
          mload(callData),
          0,
          0
      )
    }
    assert(!success);
  }

This test found in most auth tests is bound to always succeed without really testing it, actually there's no chance a call to this function would reach the assembly part.

Example: if the fuzzer parses in 0xb10e2d527612073b26eecdfd717e6a320cf44b4afac2b0732d9fcbe2b7fa0cf6, the array is filled up with the function sigs the contract has but then, firstly this line bytes8 func_selector = bytes8(callData >> 0xe0); is bound to return 0x0000000000000000. Using the bytes32 above this will firstly execute (0xb10e2d527612073b26eecdfd717e6a320cf44b4afac2b0732d9fcbe2b7fa0cf6 >> 0xe0) which is 0xb10e2d52 with 224 bits of zero to the left. Performing bytes8(0xb10e2d52) will only take the first 8 bytes of this which is all zeroes.

I fixed it in assembly with

bytes4 func_selector;
        assembly {
            func_selector := and(
                callData,
                0xffffffff00000000000000000000000000000000000000000000000000000000
            )
        }

Secondly, even if the above issue didn't exist, the if statement states that if (func_selector != func_selectors[i]) { return; } while looping through the array of signatures. The issue is that rather than checking that if the shifted bytes above returns the same bytes as a function sig in the array it should return as that's not the aim of the test but rather it returns if the shifted bytes is different from any one in the array meaning that it would only get to the assembly part if the sig is the same and which will obviously pass. The fix was to change != to ==

The third issue was the assembly part. It wasn't detected i believe because the test never got there but if it did the execution would have ran out of gas. In the call it asks the evm to read from add(callData, OneWord) which could be as much as uint256.max + 0x80 also it asks it to read up to the latter plus mload(calldata) which could be doubling the size minus 0x80.

The fix was this

bool success = false;
        assembly {
            mstore(0x80, callData)
            // if its a state changing call this will return 0
            success := staticcall(gas(), target, 0x80, 0x20, 0, 0)

            if iszero(success) {
                success := call(gas(), target, 0, 0x80, 0x20, 0, 0)
            }
        }

Checking a static call first incase which if it fails we make a call. mstoring calldata in memory and using it.

The last part involveed adding config2 and getConfig2Address to foundry-huff to work. TSOwnable failed because the owner couldn't be determined deterministically so create2 could have worked but then it wasn;t available in foundry-huff

To make this work just add

function config2(uint256 salt) public returns (HuffConfig) {
        return new HuffConfig{salt: bytes32(salt)}();
    }

    function getConfig2Address(uint256 salt) public view returns (address) {
        return
            address(
                uint160(
                    uint256(
                        keccak256(
                            abi.encodePacked(
                                bytes1(0xff),
                                address(this),
                                bytes32(salt),
                                keccak256(type(HuffConfig).creationCode)
                            )
                        )
                    )
                )
            );
    }

to foundry-huff in lib/foundry-huff/src/HuffDeployer.sol

A pull request with tests to this (basically dups of the create tests) is in foundry-huff too.

Update: Found a rather simple trick to solving the TSOwnable failing test without a need to add config2, by separating the HuffConfig deployment and storing the returned address.

[devtooligan]

hey hey, thanks so much for your contribution. this looks really cool!

could you please remove the white space changes that probably your linter added?

as you know we are volunteer run and currently quite behind on pr reviews. the additional white space changes here make it a lot harder to see your new diff.

i know we are inconsistent throughout this codebase with the whitespace, if you have an opinion on that or would like to fix some of that, then please submit that in a separate pr.

thanks fren!!

[devtooligan]

also we need to determine if this should be merged to main or dev:

#88 (comment)