Solidity: abi.decode does not decode selector

[3esmit]

Description

I am trying to update my contracts from older solidity, and I want to benefit from abi.decode.

However, the abi.decode fails within my data type (bytes4, bytes32, address, bytes32, bytes32) which is encoded with selector.

Environment

• Compiler version: all that support abi.decode

Steps to Reproduce

pragma solidity >=0.4.22 <0.7.0;

contract AbiDecodeTest {
    // use _data as 0xb82fedbbee8ea5499c660b8322896ada651fc8774b4a23e582b75e8f8e51d629da556a7400000000000000000000000035d76d9fad7c95873266deffd2be50ce2c350c37dbb31252d9bddb4e4d362c7b9c80cba74732280737af97971f42ccbdc716f3f3efb1db366880e52d09b1bfd59842e833f3004088892b7d14b9ce9e957cea9a82

//The function below works fine
    function abiDecodeTest(
        bytes memory _data
    )
        public
        pure
        returns(
            bytes4 sig,
            bytes32 label,
            address account,
            bytes32 pubkeyA,
            bytes32 pubkeyB
        )
    {
        assembly {
            sig := mload(add(_data, 32))
            label := mload(add(_data, 36))
            account := mload(add(_data, 68))
            pubkeyA := mload(add(_data, 100))
            pubkeyB := mload(add(_data, 132))
        }
    }

//The function below reverts
    function abiDecodeTest2(
        bytes memory _data
    )
        public
        pure
        returns(
            bytes4 sig,
            bytes32 label,
            address account,
            bytes32 pubkeyA,
            bytes32 pubkeyB
        )
    {
        (sig, label, account, pubkeyA, pubkeyB) = abi.decode(_data, (bytes4, bytes32, address, bytes32, bytes32));
    }
}

[chriseth]

abi.decode is the converse of abi.encode. From the fact that you are using bytes4 sig I assume that you are trying to do abi decoding and function selector decoding at the same time. This won't work because the abi encoding pads all types to 32 bytes, while the function selector is not padded.

If you change _data to bytes calldata, then you can use the following to decode: abi.decode(_data[4:], (bytes32, address, bytes32, bytes32));

[3esmit]

Thanks for the fast response and for enlightening my issue. Yes, that's exactly what I am trying to do.

In case I need to check the sig, how could I extract the bytes4? Maybe is a case of having a decodeWithSignature?

As this looks like a common use case, perhaps some "note" in documentation would be helpful.

[axic]

msg.sig can be used to acquire the signature.

[3esmit]

msg.sig can be used to acquire the signature.

Not for my usecase, because the _data being decoded is passed as a parameter, and msg.sig is related to approveAndCall.
See:

    /**
     * @notice Support for "approveAndCall". Callable only by `token()`.
     * @param _from Who approved.
     * @param _amount Amount being approved, need to be equal `getPrice()`.
     * @param _token Token being approved, need to be equal `token()`.
     * @param _data Abi encoded data with selector of `register(bytes32,address,bytes32,bytes32)`.
     */
    function receiveApproval(
        address _from,
        uint256 _amount,
        address _token,
        bytes calldata _data
    )
        public
        override
    {
        require(_amount == price, "Wrong value");
        require(_token == address(token), "Wrong token");
        require(_token == address(msg.sender), "Wrong call");
        require(_data.length <= 132, "Wrong data length");  
        require(
            getSig(_data) == this.register.selector,
            "Wrong method selector"
        );
        (bytes32 label, address account, bytes32 pubkeyA, bytes32 pubkeyB) = abi.decode(_data[4:], (bytes32, address, bytes32, bytes32));
        registerUser(_from, label, account, pubkeyA, pubkeyB);
    }

    function getSig(
        bytes memory _data
    ) 
        private 
        pure 
        returns(
            bytes4 sig
        ) 
    {
        assembly {
            sig := mload(add(_data, 32))
        }
    }

[axic]

If you use bytes calldata data as suggested by @chriseth then you can slice it data[0:4] (but there's still no support for converting bytes to bytes4).

This issue came up multiple times, not sure if we should consider introducing decodeWithSelector:

(sig, ...) = abi.decodeWithSelector(data, (...));

where the signature is not included in the type list on the right, but is returned.

[3esmit]

I always have to use receiveApproval, so we don't have to use two transactions to pay from ERC20, and also we don't have to make a "approveAll" (which recently caused a security bug in a DeFi project).

For this example I posed above, the contract happens to only have one function that is "payable in ERC20", but that's not always the case, so extracting the sig is very important. In the example above I only want to make sure that the sig is passed correctly.

Within receiveApproval context, for other project I have a use-case where the selector would select the function being called internally.

The way I coded this is to make use of .encodeABI(), where the function accepts the normal separated call of token.approve(value,myContract) and myContract.register() but also the token.approveAndCall(value,myContract,register().encodeABI()).

Anyway, the support of abi.decode with _data[4:] greatly enhanced the way we do this, and having the abi.decodeWithSelector would make things even better, however if solidity support a casting of byte[4] to bytes4 would also be a nice solution. Right now I need this auxiliary function getSig(bytes memory) to extract the selector.

[axic]

however if solidity support a casting of byte[4] to bytes4

Well you can implement a simple helper for it:

bytes4 ret = data[0] << 24 | data[1] << 16 | data[2] << 8 | data[3]

(I may have confused byte order, but you get the gist of it)

[axic]

abi.decodeWithSelector was actually originally proposed, with the semantics I suggest above: https://github.com/ethereum/solidity/issues/3876#issuecomment-380894683

Not sure why it wasn't implemented.

[3esmit]

bytes4 ret = data[0] << 24 | data[1] << 16 | data[2] << 8 | data[3]

That's very neat! Thanks for the tip!

abi.decodeWithSelector was actually originally proposed, with the semantics I suggest above: #3876 (comment)

Seems like no one opposed it, so perhaps it was just left to be implemented later?

[3esmit]

@axic the byte shifting strategy works nice, here a test contract to compare all decoding costs:
And it's bytes4 sig = _data[0] | bytes4(_data[1]) >> 8 | bytes4(_data[2]) >> 16 | bytes4(_data[3]) >> 24;, it needs to cast to bytes4 before shifitng, otherwise byte will just overflow. Also it should be shifted to the other side and direction. I'll use this strategy while the "decodeWithSelector" is not implemented.

pragma solidity >=0.4.22 <0.7.0;

contract AbiDecodeTest {
    //test data: 0xb82fedbbee8ea5499c660b8322896ada651fc8774b4a23e582b75e8f8e51d629da556a7400000000000000000000000035d76d9fad7c95873266deffd2be50ce2c350c37dbb31252d9bddb4e4d362c7b9c80cba74732280737af97971f42ccbdc716f3f3efb1db366880e52d09b1bfd59842e833f3004088892b7d14b9ce9e957cea9a82

    //execution cost: 860 gas
    function abiDecodeTest(
        bytes memory _data
    )
        public
        pure
        returns(
            bytes4 sig,
            bytes32 label,
            address account,
            bytes32 pubkeyA,
            bytes32 pubkeyB
        )
    {
        assembly {
            sig := mload(add(_data, 32))
            label := mload(add(_data, 36))
            account := mload(add(_data, 68))
            pubkeyA := mload(add(_data, 100))
            pubkeyB := mload(add(_data, 132))
        }
    }

    //execution cost: 1294 gas 
    function abiDecodeTest2(
        bytes calldata _data
    )
        external
        pure
        returns(
            bytes4 sig,
            bytes32 label,
            address account,
            bytes32 pubkeyA,
            bytes32 pubkeyB
        )
    {
        sig = _data[0] |  bytes4(_data[1]) >> 8 | bytes4(_data[2]) >> 16  | bytes4(_data[3]) >> 24;
        (label, account, pubkeyA, pubkeyB) = abi.decode(_data[4:], (bytes32, address, bytes32, bytes32));
    }

    //execution cost: 1192 gas
    function abiDecodeTest3(
        bytes calldata _data
    )
        external
        pure
        returns(
            bytes4 sig,
            bytes32 label,
            address account,
            bytes32 pubkeyA,
            bytes32 pubkeyB
        )
    {
        sig = getSelector(_data);
        (label, account, pubkeyA, pubkeyB) = abi.decode(_data[4:], (bytes32, address, bytes32, bytes32));
    }

    function getSelector(bytes memory _data) private pure returns(bytes4 sig) {
        assembly {
            sig := mload(add(_data, 32))
        }
    }

    //execution cost: 1885 gas
    function abiDecodeTest4(
        bytes calldata _data
    )
        external
        pure
        returns(
            bytes4 sig,
            bytes32 label,
            address account,
            bytes32 pubkeyA,
            bytes32 pubkeyB
        )
    {
        sig = getSelector(_data);
        (label, account, pubkeyA, pubkeyB) = abi.decode(slice(_data,4,_data.length-4), (bytes32, address, bytes32, bytes32));
    }

    function getSelector(bytes memory _data) private pure returns(bytes4 sig) {
        assembly {
            sig := mload(add(_data, 32))
        }
    }

    function slice(bytes memory _bytes, uint _start, uint _length) private pure returns (bytes memory) {
        require(_bytes.length >= (_start + _length));

        bytes memory tempBytes;

        assembly {
            switch iszero(_length)
            case 0 {
                // Get a location of some free memory and store it in tempBytes as
                // Solidity does for memory variables.
                tempBytes := mload(0x40)

                // The first word of the slice result is potentially a partial
                // word read from the original array. To read it, we calculate
                // the length of that partial word and start copying that many
                // bytes into the array. The first word we copy will start with
                // data we don't care about, but the last `lengthmod` bytes will
                // land at the beginning of the contents of the new array. When
                // we're done copying, we overwrite the full first word with
                // the actual length of the slice.
                let lengthmod := and(_length, 31)

                // The multiplication in the next line is necessary
                // because when slicing multiples of 32 bytes (lengthmod == 0)
                // the following copy loop was copying the origin's length
                // and then ending prematurely not copying everything it should.
                let mc := add(add(tempBytes, lengthmod), mul(0x20, iszero(lengthmod)))
                let end := add(mc, _length)

                for {
                    // The multiplication in the next line has the same exact purpose
                    // as the one above.
                    let cc := add(add(add(_bytes, lengthmod), mul(0x20, iszero(lengthmod))), _start)
                } lt(mc, end) {
                    mc := add(mc, 0x20)
                    cc := add(cc, 0x20)
                } {
                    mstore(mc, mload(cc))
                }

                mstore(tempBytes, _length)

                //update free-memory pointer
                //allocating the array padded to 32 bytes like the compiler does now
                mstore(0x40, and(add(mc, 31), not(31)))
            }
            //if we want a zero-length slice let's just return a zero-length array
            default {
                tempBytes := mload(0x40)

                mstore(0x40, add(tempBytes, 0x20))
            }
        }

        return tempBytes;
    }


}

[3esmit]

BTW, I am not sure if decodeWithSelector is very useful to have the "selector being returned"... Because when I know the selector, I also know the data type, so first I need to get the selector and then decode.

[axic]

I think the preferred strategy should be:

        require(bytes4(_data[:4]) == 0x12341234);
        (label, account, pubkeyA, pubkeyB) = abi.decode(_data[4:], (bytes32, address, bytes32, bytes32));

Or

     (label, account, pubkeyA, pubkeyB) = abi.decodeWithSelector(_data, 0x12341234, (bytes32, address, bytes32, bytes32));

Where 0x12341234 is the expected selector.

Please not, neither of the above works currently, but you can do the shift trick.

[3esmit]

        require(bytes4(_data[:4]) == 0x12341234);
        (label, account, pubkeyA, pubkeyB) = abi.decode(_data[4:], (bytes32, address, bytes32, bytes32));

is more versatile.

[axic]

Could also consider

     (label, account, pubkeyA, pubkeyB) = abi.decodeWithSignature(_data, "getSomething", (bytes32, address, bytes32, bytes32));

where selector is calculated based on the name and types, just like encodeWIthSignature works.