code423n4 commented 1 year ago

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/68f20cbbf24ac42d2a54660c70b0a74301bee55b/contracts/Multicall/Multicall3.sol#L43

Vulnerability details

In the Multicall3 contract, there is a function called multicall() that supports multicall without value. However, this function still has the payable modifier, which is unnecessary. If users accidentally send ETH along with this function, it will be locked forever since there is no function in the contract to consume or withdraw the ETH balance.

The contract uses multicallValue() to sum up all value used in calls and checks at the end that it equals to msg.value. This means that multicallValue() will never use the ETH balance of the contract, but only use the msg.value sent by users.

Impact

Sending ETH along with the multicall() function will result in the ETH being locked forever in the contract.

Proof of Concept

Function multicall() does not send any value when doing call to calli.target.

function multicall(
    Call[] calldata calls
) public payable returns (Result[] memory returnData) {
    uint256 length = calls.length;
    returnData = new Result[](length);
    Call memory calli;
    for (uint256 i = 0; i < length; ) {
        Result memory result = returnData[i];
        calli = calls[i];

        (result.success, result.returnData) = calli.target.call(
            calli.callData
        );
        if (!result.success) {
            _getRevertMsg(result.returnData);
        }
        unchecked {
            ++i;
        }
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Consider removing payable in the multicall() function.

Assessed type

Payable

c4-pre-sort commented 1 year ago

minhquanym marked the issue as duplicate of #1024

c4-judge commented 1 year ago

dmvt changed the severity to QA (Quality Assurance)