mds1 / multicall

Multicall: Aggregate multiple constant function call results into one
https://multicall3.com
MIT License
903 stars 150 forks source link

Intended overflow of `valAccumulator` is possible in `aggregate3Value` #195

Closed algys closed 6 months ago

algys commented 6 months ago

There is a comment in the aggregate3Value method regarding valAccumulator overflow, it says that it will never overflow, however, in case of multiple calls with a huge calli.value and allowFailure=true the overflow is simply triggerable since the call will return false success flag that is ignored but valAccumulator is still increasing after each faulty call, so user can put an arbitrary huge calli.value to trigger the overflow and bypass msg.value = SUM(call[0...i].value) check.

https://github.com/mds1/multicall/blob/main/src/Multicall3.sol#L137-L145

uint256 val = calli.value;
// Humanity will be a Type V Kardashev Civilization before this overflows - andreas
// ~ 10^25 Wei in existence << ~ 10^76 size uint fits in a uint256
unchecked { valAccumulator += val; }
(result.success, result.returnData) = calli.target.call{value: val}(calli.callData);
assembly {
    // Revert if the call fails and failure is not allowed
    // `allowFailure := calldataload(add(calli, 0x20))` and `success := load(result)`
    if iszero(or(calldataload(add(calli, 0x20)), mload(result))) {

It's not a really serious bug but the comment leads to the wrong assumption about overflow also I think if any native coin is remaining in the contract it can be drained.

mds1 commented 6 months ago

Thanks for reporting. This is a known issue (you can read more here) which is not a problem during normal usage but is a problem if you are relying on the contract to hold ETH, which is not intended usage of the contract anyway. Consequently the first bullet in the Security section of the README says

Ensure it NEVER holds funds after a transaction ends. Any ETH, tokens, or other funds held by this contract can be stolen. There are bots that monitor for this and they will immediately steal any funds they find.

Going to close this I don't think there's anything actionable here, but open to suggestions :)