Reentrancy in callBatched

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/Caller.sol#L185-L192 https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/Caller.sol#L193-L200 https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/Caller.sol#L202-L208

Vulnerability details

Proof of Concept

The Caller contract implements callBatched function in order to execute a batch of calls within one call. The function has payable declaration to be able to send ETH inside the call.

The NATSPEC is also provided in parallel;

/// @notice Executes a batch of calls. /// The caller will be set as the message sender of all the calls as per ERC-2771. /// Reverts if any of the calls reverts or any of the called addresses is not a smart contract. /// This function is payable, any Ether sent to it can be used in the batched calls.<-------------------- /// Any unused Ether will stay in this contract, /// anybody will be able to use it in future calls to callBatched. /// @param calls The calls to perform. /// @return returnData The data returned by each of the calls.

Link

Accordingly, calls parameters are provided as Call[] and the function internally calls the _call function inside the iteration;

    function callBatched(Call[] memory calls) public payable returns (bytes[] memory returnData) {
        returnData = new bytes[](calls.length);
        address sender = _msgSender();
        for (uint256 i = 0; i < calls.length; i++) {
            Call memory call = calls[i];
            returnData[i] = _call(sender, call.to, call.data, call.value);
        }
    }

Link

_call function is being called internally;

    function _call(address sender, address to, bytes memory data, uint256 value)
        internal
        returns (bytes memory returnData)
    {
        // Encode the message sender as per ERC-2771
        return Address.functionCallWithValue(to, abi.encodePacked(data, sender), value);
    }

Link

As you can see, the _call function also calls Address contract's functionCallWithValue inherited from Open Zeppelin.

functionCallWithValue is an overloaded function and it ends with;

    function functionCallWithValue(
        address target,
        bytes memory data,
        uint256 value,
        string memory errorMessage
    ) internal returns (bytes memory) {
        require(address(this).balance >= value, "Address: insufficient balance for call");
        (bool success, bytes memory returndata) = target.call{value: value}(data);
        return verifyCallResultFromTarget(target, success, returndata, errorMessage);
    }

As can be seen, it checks whether the contract has sufficient balance in order to execute the low-level call to the target with target.call{value: value}(data)

However, there is one glitch at this point. Since the calls can be made to arbitrary addresses, an organized actor can create re-entrancy by simply sending funds through this function and triggering the fallback function of his/her contract.

So once the ETH is received in the attacker contract(A), the fallback function of (A) is triggered and it calls the Caller.callBatched again, and the whole function executes again with supplied params. This results in draining the forwarder contract/wallet.

Impact

Draining funds from the forwarder wallet/contract

Tools Used

Manual Review

Recommended Mitigation Steps

The function could have the nonReentrant modifier that locks the function state during the call.

[c4-sponsor]

xmxanuel marked the issue as disagree with severity

[CodeSandwich]

[dispute vaildity] This is expected behavior. The sender of the top-level batch provides some Ether for the batch and is responsible for deciding who to call and what will be the consequences. The author sender can't lose more Ether than they've provided in the beginning. On top of that if there's not enough Ether to perform all the steps of the batch, the whole transaction reverts. The reentrancy pattern you've shown may also be used for cleaning up leftover tokens if for whatever reason they need to be sent to Caller and the batch can't be designed to always consume all of them, no only Ether but also ERC-20 tokens.

[c4-sponsor]

xmxanuel marked the issue as sponsor disputed

[GalloDaSballo]

I fail to see any state changes being performed after the call, meaning that re-entering wouldn't cause any inconsistent state

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof