RawCall builtin function allows passing a value in unsupported calls

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-09-vyper-compiler/blob/main/vyper/builtins/functions.py#L1100

Summary

Vyper compiler allows passing a value in builtin raw_call even if the call is a delegatecall or staticcall. This is very dangerous in implementations such as multicall. As a real world example, the popular snekmate library multicall util is vulnerable due to the insufficient checks in the vyper compiler

Vulnerability Details

The RawCall handler of the builtin function does not check that if value is passed to the builtin function and is_delegate_call or is_static_call is true: https://github.com/Cyfrin/2023-09-vyper-compiler/blob/main/vyper/builtins/functions.py#L1100

class RawCall(BuiltinFunction):
---------
    def build_IR(self, expr, args, kwargs, context):
---------
        gas, value, outsize, delegate_call, static_call, revert_on_failure = (
            kwargs["gas"],
            kwargs["value"],
            kwargs["max_outsize"],
            kwargs["is_delegate_call"],
            kwargs["is_static_call"],
            kwargs["revert_on_failure"],
        )
---------
        if delegate_call:
            call_op = ["delegatecall", gas, to, *common_call_args] # @audit should check that if is_delegate_call then value == 0 
        elif static_call:
            call_op = ["staticcall", gas, to, *common_call_args] # @audit should check that if is_static_call then value == 0 
        call_ir += [call_op]
---------
            return IRnode.from_list(call_ir, typ=typ)

Here is an example implementation in vyper that will be successfully compiled and deployed:

event logUint256:
    logged_uint256: indexed(uint256)

@external
@payable
def delegatedTo1():
    log logUint256(msg.value)

@external
@payable
def delegatedTo2():
    log logUint256(msg.value)

@external
@payable
def delegateToSelf():
    return_data: Bytes[300] = b""
    call_data1: Bytes[100] = _abi_encode(b"",method_id=method_id("delegatedTo1()"))
    call_data2: Bytes[100] = _abi_encode(b"",method_id=method_id("delegatedTo2()"))

    return_data = raw_call(self, call_data1, max_outsize=255, is_delegate_call=True, value=msg.value/2)
    return_data = raw_call(self, call_data2, max_outsize=255, is_delegate_call=True, value=msg.value/2)

In the above example, the developer would expect to receive the passed msg.value/2 in delegatedTo1/2 however they would receive the full msg.value

Transaction trace when sending 100 to delegateToSelf shows that both delegatecalls output 100(0x64) instead of 50:

    function test_incorrectMsgValueDelegatecall() external {
        address vyper = address(0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0);
        vyper.call{value: 100}(abi.encodeWithSignature("delegateToSelf()"));
    }

--------

Running 1 test for test/Counter.t.sol:CounterTest
[PASS] test_incorrectMsgValueDelegatecall() (gas: 13858)
Traces:
  [13858] CounterTest::test_incorrectMsgValueDelegatecall() 
    ├─ [3956] 0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0::delegateToSelf{value: 100}() 
    │   ├─ [27] PRECOMPILE::identity(0x) [staticcall]
    │   │   └─ ← 0x
    │   ├─ [27] PRECOMPILE::identity(0x) [staticcall]
    │   │   └─ ← 0x
    │   ├─ [1221] 0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0::541c930c(00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000000) [delegatecall]
    │   │   ├─  emit topic 0: 0xd74736c81b9d709d9d3cc16b682a1075c6b99b57b848fefb07ba5368ff27827d
    │   │   │       topic 1: 0x0000000000000000000000000000000000000000000000000000000000000064
    │   │   │           data: 0x
    │   │   └─ ← ()
    │   ├─ [18] PRECOMPILE::identity(0x) [staticcall]
    │   │   └─ ← 0x
    │   ├─ [1221] 0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0::f0b781bd(00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000000) [delegatecall]
    │   │   ├─  emit topic 0: 0xd74736c81b9d709d9d3cc16b682a1075c6b99b57b848fefb07ba5368ff27827d
    │   │   │       topic 1: 0x0000000000000000000000000000000000000000000000000000000000000064
    │   │   │           data: 0x
    │   │   └─ ← ()
    │   ├─ [18] PRECOMPILE::identity(0x) [staticcall]
    │   │   └─ ← 0x
    │   └─ ← ()
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 186.29ms

In solidity however this is not possible. The compiler would throw an error:

pragma solidity 0.8.17;

contract SolidityDelegatecallValue {
    function tryMe() external {
        (bool succeess, bytes memory retVal) = address(this).delegatecall{value: 100}("");
    }

    receive() external payable {}
}

When compiling:

Error: 
Compiler run failed:
Error (6189): Cannot set option "value" for delegatecall.
 --> src/solidity_delegatecall_value.sol:5:48:
  |
5 |         (bool succeess, bytes memory retVal) = address(this).delegatecall{value: 100}("");
  |                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Impact

This allows developers to perform a delegatecall or staticcall with a value that will not be used because of the nature of delegatecall and staticcall. This can disrupt accounting and easily be missed by developers thus causing a loss of funds

This would be extremely proplematic in multicall implementations. A real world example is the implementation of the popular snekmate libraries: https://github.com/pcaversaccio/snekmate/blob/5fe40ea7376b0405244d6c3f4f4f6c7b047c146b/src/utils/Multicall.vy#L169

@external
@payable
def multicall_value_self(data: DynArray[BatchValueSelf, max_value(uint8)]) -> DynArray[Result, max_value(uint8)]:
------
    value_accumulator: uint256 = empty(uint256)
    results: DynArray[Result, max_value(uint8)] = []
    return_data: Bytes[max_value(uint8)] = b""
    success: bool = empty(bool)
    for batch in data:
        msg_value: uint256 = batch.value
        value_accumulator = unsafe_add(value_accumulator, msg_value)
        if (batch.allow_failure == False):
            return_data = raw_call(self, batch.call_data, max_outsize=255, value=msg_value, is_delegate_call=True)
            success = True
            results.append(Result({success: success, return_data: return_data}))
        else:
            success, return_data = \
                raw_call(self, batch.call_data, max_outsize=255, value=msg_value, is_delegate_call=True, revert_on_failure=False)
            results.append(Result({success: success, return_data: return_data}))
    assert msg.value == value_accumulator, "Multicall: value mismatch"
    return results

Tools Used

Foundry, Vyper

Recommendations

Throw an exception RawCall.build_ir in builtins/functions.py if value is not 0 and is_delegate_call or is_static_call are true.