External calls can overflow return data to return input buffer

Severity

High Risk

Relevant GitHub Links

https://github.com/vyperlang/vyper/blob/9ce56e7d8b0196a5d51d706a8d2376b98d3e8ad7/vyper/codegen/external_call.py#L33-L142

Summary

When calls to external contracts are made, we write the calldata starting at byte 28, and allocate the return buffer to start at byte 0 (overlapping with the calldata). When checking RETURNDATASIZE for dynamic types, the size is compared only to the minimum allowed size for that type, and not to the returned value's length. As a result, malformed return data can cause the contract to mistake its own calldata for returndata.

Vulnerability Details

When arguments are packed for an external call, we create a buffer of size max(args, return_data) + 32. The calldata is placed in this buffer (starting at byte 28), and the return buffer is allocated to start at byte 0. The assumption is that we can reuse the memory becase we will not be able to read past RETURNDATASIZE.

if fn_type.return_type is not None:
    return_abi_t = calculate_type_for_external_return(fn_type.return_type).abi_type

    # we use the same buffer for args and returndata,
    # so allocate enough space here for the returndata too.
    buflen = max(args_abi_t.size_bound(), return_abi_t.size_bound())
else:
    buflen = args_abi_t.size_bound()

buflen += 32  # padding for the method id

When data is returned, we unpack the return data by starting at byte 0. We check that RETURNDATASIZE is greater than the minimum allowed for the returned type:

if not call_kwargs.skip_contract_check:
    assertion = IRnode.from_list(
        ["assert", ["ge", "returndatasize", min_return_size]],
        error_msg="returndatasize too small",
    )
    unpacker.append(assertion)

This check ensures that any dynamic types returned will have a size of at least 64. However, it does not verify that RETURNDATASIZE is as large as the length word of the dynamic type.

As a result, if a contract expects a dynamic type to be returned, and the part of the return data that is read as length includes a size that is larger than the actual RETURNDATASIZE, the return data read from the buffer will overrun the actual return data size and read from the calldata.

Proof of Concept

This contract calls an external contract with two arguments. As the call is made, the buffer includes:

• byte 28: method_id
• byte 32: first argument (0)
• byte 64: second argument (hash)

The return data buffer begins at byte 0, and will return the returned bytestring, up to a maximum length of 96 bytes.

interface Zero:
    def sneaky(a: uint256, b: bytes32) -> Bytes[96]: view

@external
def test_sneaky(z: address) -> Bytes[96]:
    return Zero(z).sneaky(0, keccak256("oops"))

On the other side, imagine a simple contract that does not, in fact, return a bytestring, but instead returns two uint256s. I've implemented it in Solidity for ease of use with Foundry:

function sneaky(uint a, bytes32 b) external pure returns (uint, uint) {
    return (32, 32);
}

The return data will be parsed as a bytestring. The first 32 will point us to byte 32 to read the length. The second 32 will be perceived as the length. It will then read the next 32 bytes from the return data buffer, even though those weren't a part of the return data.

Since these bytes will come from byte 64, we can see above that the hash was placed there in the calldata.

If we run the following Foundry test, we can see that this does in fact happen:

function test__sneakyZeroReturn() public {
    ZeroReturn z = new ZeroReturn();
    c = SuperContract(deployer.deploy("src/loose/", "ret_overflow", ""));
    console.logBytes(c.test_sneaky(address(z)));
}

Logs:
  0xd54c03ccbc84dd6002c98c6df5a828e42272fc54b512ca20694392ca89c4d2c6

Impact

Malicious or mistaken contracts returning the malformed data can result in overrunning the returned data and reading return data from the calldata buffer.

Tools Used

Manual Review, Foundry

Recommendations

If we want to continue to use the same buffer for calldata and return data, add an additional safety check for dynamic return types that the RETURNDATASIZE is checked against the bytes that will be unpacked as the length.

Alternatively, allocate the return data buffer separately in memory.