Solidity: [Abiv2 proto fuzzer] Use variable length literals to initialize arguments of `bytes`/s

[bshastry]

Abstract

In #7177, after taking a look at @ekpyron (see https://github.com/ethereum/solidity/pull/7177#discussion_r310948711), I feel that the current approach that the proto fuzzer takes to initialize arguments of type bytes/string is insufficient to test abi coding.

Currently, the proto fuzzer always initializes a bytes argument with a value that is exactly 32 bytes in size.

Ideally, we should be using a variable length literal to initialize an argument of bytes/string type.

Motivation

Presently, the routine that provides the literal used to initialize a bytes or a string type variable is this

https://github.com/ethereum/solidity/blob/cd563e526a92721a1e9e1633f5e2c9e53e77b242/test/tools/ossfuzz/protoToAbiV2.h#L242-L245

The routine hashes a monotonically increasing counter using keccak256 and assigns the resulting 32 byte hash value (in hex encoding minus the 0x prefix) to a variable of bytes or string type. The initialized value is a string literal (that holds the hash value).

TBD.

Specification

Backwards Compatibility

[ekpyron]

We should remember to check whether this will prompt padding issues in the creation of invalid (cropped) encodings.

[bshastry]

Just verified that cropping fewer bytes incorrectly leads to successful execution of low level call. Example test case is the following which fails the check encoded in these lines of solidity code

        // Return non-zero value if call succeeds for incorrect encoding
        if (success == true)
            return 400001;

pragma solidity >=0.0;
pragma experimental ABIEncoderV2;

contract C {

    bytes x_0;

    function test() public returns (uint) {

        x_0 = hex"";
        uint returnVal = this.coder_public(x_0);
        if (returnVal != 0)
            return returnVal;

        returnVal = this.coder_external(x_0);
        if (returnVal != 0)
            return uint(200000) + returnVal;

        bytes memory argumentEncoding = abi.encode(x_0);

        returnVal = checkEncodedCall(this.coder_public.selector, argumentEncoding, 67);
        if (returnVal != 0)
            return returnVal;

        returnVal = checkEncodedCall(this.coder_external.selector, argumentEncoding, 67);
        if (returnVal != 0)
            return uint(200000) + returnVal;
        return 0;
    }

    function bytesCompare(bytes memory a, bytes memory b) internal pure returns (bool) {
        if(a.length != b.length)
            return false;
        for (uint i = 0; i < a.length; i++)
            if (a[i] != b[i])
                return false;
        return true;
    }

    /// Accepts function selector, correct argument encoding, and length of invalid encoding and returns
    /// the correct and incorrect abi encoding for calling the function specified by the function selector.
    function createEncoding(bytes4 funcSelector, bytes memory argumentEncoding, uint invalidLengthFuzz)
        internal pure returns (bytes memory, bytes memory)
    {
        bytes memory validEncoding = new bytes(4 + argumentEncoding.length);
        // Ensure that invalidEncoding crops at least one and at most all bytes from correct encoding.
        // Check if shorter bytes/string values don't lead to successful decoding even when encoding
        // is cropped.
        uint invalidLength = invalidLengthFuzz % argumentEncoding.length + 10;
        bytes memory invalidEncoding = new bytes(4 + invalidLength);
        for (uint i = 0; i < 4; i++)
            validEncoding[i] = invalidEncoding[i] = funcSelector[i];
        for (uint i = 0; i < argumentEncoding.length; i++)
            validEncoding[i+4] = argumentEncoding[i];
        for (uint i = 0; i < invalidLength; i++)
            invalidEncoding[i+4] = argumentEncoding[i];
        return (validEncoding, invalidEncoding);
    }

    /// Accepts function selector, correct argument encoding, and an invalid encoding length as input.
    /// Returns a non-zero value if either call with correct encoding fails or call with incorrect encoding
    /// succeeds. Returns zero if both calls meet expectation.
    function checkEncodedCall(bytes4 funcSelector, bytes memory argumentEncoding, uint invalidLengthFuzz) public returns (uint)
    {
        (bytes memory validEncoding, bytes memory invalidEncoding) = createEncoding(funcSelector, argumentEncoding, invalidLengthFuzz);
        (bool success, bytes memory returnVal) = address(this).call(validEncoding);
        uint returnCode = abi.decode(returnVal, (uint));
        // Return non-zero value if call fails for correct encoding
        if (success == false || returnCode != 0)
            return 400000;
        (success, ) = address(this).call(invalidEncoding);
        // Return non-zero value if call succeeds for incorrect encoding
        if (success == true)
            return 400001;
        return 0;
    }

    function coder_public(bytes memory c_0) public pure returns (uint) {

        if (!bytesCompare(c_0, hex"")) return 1;
        return 0;
    }

    function coder_external(bytes calldata c_0) external pure returns (uint) {

        if (!bytesCompare(c_0, hex"")) return 1;
        return 0;
    }

}

[bshastry]

@ekpyron I am working on a more precise setting for invalid encoding length. Initially, I
thought of doing something like the following (inside solidity test program template)

        uint invalidLength = invalidLengthFuzz % argumentEncoding.length;
        // Ensure that invalidEncoding crops at least 32 bytes since shorter
        // bytes/string values can lead to successful decoding even when fewer
                // than 32 bytes have been cropped.
        if (invalidLength < 32)
            invalidLength = 32;

but then, this never leads to an invalid encoding that is less than 32 bytes long.

At the moment, I am considering something like

        uint invalidLength = invalidLengthFuzz % argumentEncoding.length;
        // Ensure that invalidEncoding crops at least 32 bytes when creating
                // an invalid encoding for a (correct) encoding whose length is less
                // than or equal to some predefined threshold that is indicative of 
                // unused padded bytes
        if (argumentEncoding.length <= <some_threshold_to_be_decided> && invalidLength < 32)
            invalidLength = 32;

do you think this makes sense?

[ekpyron]

What about

uint invalidLength = invalidLengthFuzz % (argumentEncoding.length - 31)

?
That is, if argumentEncoding.length is always greater than 31 - otherwise maybe something like

uint invalidLength = (argumentEncoding.length > 31) ? (invalidLengthFuzz % (argumentEncoding.length - 31)) : (argumentEncoding.length /2);

?

Padding will be at most 31 bytes long, so if invalidLenth < argumentEncoding.length - 31 we should be fine I think.

[bshastry]

I will test this a bit and ping back

uint invalidLength = (argumentEncoding.length > 31) ? (invalidLengthFuzz % (argumentEncoding.length - 31)) : (invalidLengthFuzz % argumentEncoding.length);

I modified the false case just so we get a somewhat uniform distribution over 0--31. Don't know if it matters though. Thank you for your feedback :-)

[ekpyron]

Haha, I just realize: the actual problem is that the argumentEncoding is padded, so we can of course be sure that it is at least 32 bytes long ;-)!

[bshastry]

Oh okay, I'm a bit confused now after reading this from https://solidity.readthedocs.io/en/v0.5.10/abi-spec.html

The encoding of string or bytes does not apply padding at the end unless it is part of an array or struct (then it is padded to a multiple of 32 bytes).

[ekpyron]

That snippet is from "Non-standard Packed Mode".

If the encoding wasn't padded, then https://github.com/ethereum/solidity/issues/7180#issuecomment-518626780 would work just fine.

(If I'm not mistaken ;-))

[bshastry]

Doh, sorry about my last comment, I should have browsed up to the heading the comment was under!