Solidity: abi.decode cannot decode `msg.data`

[3esmit]

I'm trying to use abi.decode to decode abi encoded calls.

I noticed that this is not possible because abi.decode fails to shift out the "msg.sig", the first 4 bytes.

pragma solidity >=0.5.0 <0.6.0; 
contract Foo {
    function bar(uint256 _a, address _b) external pure returns (uint256 a, address b) {
        (a,b) = abi.decode(msg.data, (uint256,address));
    }
}

This is the result:

decoded input | {   "uint256 _a": "1",  "address _b": "0xDe4A88ef731cc55450C76E9307A64e94146006f7" }
-- | --
decoded output | {  "0": "uint256: a 43775121307266258428473763376100980315118985562653986292681922597120504758272",    "1": "address: b 0x00000000de4a88EF731cc55450C76E9307A64E94" }

I noticed that if I remove the 4 bytes from start and it decodes correctly:

pragma solidity >=0.5.0 <0.6.0; 
contract Foo {
    function bar(bytes calldata data) external pure returns (uint256 a, address b) {
        (a,b) = abi.decode(data, (uint256,address));
    }
}

Using data (with bytes4 msg.sig):

decoded input | {   "bytes data": "0xa55176590000000000000000000000000000000000000000000000000000000000000001000000000000000000000000de4a88ef731cc55450c76e9307a64e94146006f7" }
-- | --
decoded output | {  "0": "uint256: a 74775551433990177875228592555497145942085465184237639638388956040984682037248",    "1": "address: b 0x00000000de4a88EF731cc55450C76E9307A64E94" }

Cropping out the msg.sig:

decoded input | {   "bytes data": "0x0000000000000000000000000000000000000000000000000000000000000001000000000000000000000000de4a88ef731cc55450c76e9307a64e94146006f7" }
-- | --
decoded output | {  "0": "uint256: a 1",    "1": "address: b 0xDe4A88ef731cc55450C76E9307A64e94146006f7" }

I tried abi.decode(msg.data, (bytes4,uint256,address)); but then this line reverts.
Maybe we need a abi.decodePacked?

[ekpyron]

Yes, abi.decode(msg.data, (bytes4,uint256)); will not only skip the first 4 bytes, but assume that the bytes4 part is padded to 32 bytes - that's why it reverts due to the payload being too small.
This is a known limitation (although I don't think it was tracked on github before, so thanks for reporting!) and should definitely be fixed, but I'm not sure about the current plans (@chriseth, @axic ?). I'll queue the issue for discussion for the upcoming releases.

Possible solutions include indeed abi.decodePacked or a flag for abi.decode or abi.decodeWithoutSignature, another field of msg containing the payload without signature, being able to offset a calldata array to skip the first 4 bytes (like msg.data + 4), etc. (I'm not aware of any decision yet myself).
I think it'd be best not to expose a dedicated abi.* function just for this. I'm not sure whether "pointer arithmetic" on references is an extremely good or a terrible idea right now :-).

[3esmit]

So I would expect some way more convenient to decode this ABI calldata.

what about abi.decodeCall(data, expectedSig, types) where the first 4 bytes must match the expectedSig?
Such as abi.decodeCall(data, bytes4(keccak256("bar(uint256,address)")), (uint256,address))
My use case is not with msg.data, but it decodes that type of abi.

Or perhaps abi.decodeCall(data, Foo.bar(uint256,address)), so it would do the same as above, but using the Contract Type and looking for the sig/params in the Type ?

I am using "approveAndCall" of MiniMeToken.

its called like this in the js:

const buyABI = StickerMarket.methods.buyToken(packId, packBuyer).encodeABI();
const buySend = await MiniMeToken.methods.approveAndCall(StickerMarket.address, price, buyABI).send();

This is the solution I came with:

contract StickerMarket {
//(...)
   /**
     * @notice MiniMeToken ApproveAndCallFallBack forwarder for registerPack and buyToken
     * @param _from account calling "approve and buy" 
     * @param _token must be exactly SNT contract
     * @param _data abi encoded call 
     */
    function receiveApproval(
        address _from,
        uint256,
        address _token,
        bytes calldata _data
    ) 
        external 
    {
        require(_token == address(snt), "Bad token");
        require(_token == address(msg.sender), "Bad call");
        bytes4 sig = abiDecodeSig(_data); //can be replaced by abi.decode(_data, (bytes4)); 
        bytes memory cdata = slice(_data,4,_data.length-4);
        if(sig == bytes4(keccak256("buyToken(uint256,address)"))){
            require(cdata.length == 64, "Bad data length");
            (uint256 packId, address owner) = abi.decode(cdata, (uint256, address));
            buy(_from, packId, owner);
        } else if(sig == bytes4(keccak256("registerPack(uint256,uint256,bytes4[],address,bytes)"))) {
            require(cdata.length >= 156, "Bad data length");
            (uint256 _price, uint256 _donate, bytes4[] memory _category, address _owner, bytes memory _contenthash) = abi.decode(cdata, (uint256,uint256,bytes4[],address,bytes));
            register(_from, _category, _owner, _price, _donate, _contenthash);
        } else {
            revert("Bad call");
        }
    }
//(...)
    function abiDecodeSig(bytes memory _data) private pure returns(bytes4 sig){
        assembly {
            sig := mload(add(_data, add(0x20, 0)))
        }
    }

    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]

In another contract I've written, I did like this to decode the abi of call:

    /**
     * @dev Decodes abi encoded data with selector for "register(bytes32,address,bytes32,bytes32)".
     * @param _data Abi encoded data.
     * @return Decoded registry call.
     */
    function abiDecodeRegister(
        bytes _data
    ) 
        private 
        pure 
        returns(
            bytes4 sig,
            bytes32 label,
            address account,
            bytes32 pubkeyA,
            bytes32 pubkeyB
        )
    {
        assembly {
            sig := mload(add(_data, add(0x20, 0)))
            label := mload(add(_data, 36))
            account := mload(add(_data, 68))
            pubkeyA := mload(add(_data, 100))
            pubkeyB := mload(add(_data, 132))
        }
}

[chriseth]

The purpose of abi.decode is ABI decoding. msg.data is not ABI encoded, so it cannot be used there.

I think a single call to calldatacopy is a good way to solve this problem. I think introducing a new member of msg that represents only the post-signature part might be another solution.

[ekpyron]

@chriseth What about subranges for arrays? Like msg.data.subrange(4,-1) or something like that? That would be the most general solution and would be possible if we keep the array length on the stack anyways...

[3esmit]

Can we have a method for decoding msg.data.
What is the coding of msg.data called? I mean the "4bytes" + data.
It is very useful for approveAndCall pattern I have to deal with to make MiniMeToken, which uses this function with calldata embedded.
I see some ways around this, like a self call using that parameters, but this seems less "human readable" then processing it inside receivedApproval method.

[leonardoalt]

Fixed by https://github.com/ethereum/solidity/pull/7340

[k06a]

Found curious ways to avoid this issue, just prepend bytes28(0) to parse selector as bytes32:

abi.decode(
    abi.encodePacked(bytes28(0), msg.data),
    (bytes32,address,uint256)
);