code-423n4 / 2023-01-drips-findings

0 stars 2 forks source link

Caller.callAs/callSigned/callBatched does not refund the remaining ETH and these ETH can be used by others in callBatched #224

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

In the Caller.callAs/callSigned/callBatched functions, the user provides ETH to make calls to functions of other contracts.

    function callAs(address sender, address to, bytes memory data)
        public
        payable
        returns (bytes memory returnData)
    {
        require(isAuthorized(sender, _msgSender()), "Not authorized");
        return _call(sender, to, data, msg.value);
    }

If other contracts do not support ERC-2771, these contracts may recognize the Caller contract as the caller and refund the remaining ETH to the Caller contract. Since the Caller does not refund the remaining ETH to the caller, these ETH will remain in the Caller contract. Also in the Caller.callBatched function, since there is no check for msg.value == sum(call.value), this allows the user to steal the ETH left in the Caller contract in Caller.callBatched.

    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);
        }
    }

For example, the user may exchange ETH for ERC20 tokens in callBatched and then drip,and during the exchange there may a refund of unused ETH to the Caller contract.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider checking msg.value == sum(call.value) in the Caller.callBatched function

GalloDaSballo commented 1 year ago

Self-inflicted so QA Low imo

GalloDaSballo commented 1 year ago

L

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

GalloDaSballo marked the issue as grade-c