Multicall being payable is seriously dangerous.

[Amxx]

The Multicall.multicall pattern is getting a lot of love lately. I first saw it used by ENS and now in Uniswap-V3-periphery. Yet, I see that your implementation includes the payable keyword. I do believe this is potentially dangerous and should be removed. People might copy that and not see the side-effect.

Example:

pragma solidity >=0.8.2;

import "https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.0.0/contracts/token/ERC20/ERC20.sol";
import "https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.0.0/contracts/utils/Address.sol";

abstract contract Multicall {
    function multicall(bytes[] calldata data) external payable returns (bytes[] memory results) {
        results = new bytes[](data.length);
        for (uint256 i = 0; i < data.length; i++) {
            (bool success, bytes memory result) = address(this).delegatecall(data[i]);

            if (!success) {
                // Next 5 lines from https://ethereum.stackexchange.com/a/83577
                if (result.length < 68) revert();
                assembly {
                    result := add(result, 0x04)
                }
                revert(abi.decode(result, (string)));
            }

            results[i] = result;
        }
    }
}

contract VulnerableWETH is ERC20, Multicall {
    constructor() ERC20('VulnerableWETH', 'VWETH') {}
    
    receive() external payable {
        _mint(msg.sender, msg.value);
    }
    
    function deposit() external payable {
        _mint(msg.sender, msg.value);
    }
    
    function withdraw(uint256 value) external {
        _burn(msg.sender, value);
        Address.sendValue(payable(msg.sender), value);
    }
    
    function withdrawAll() external {
        uint256 value = balanceOf(msg.sender);
        _burn(msg.sender, value);
        Address.sendValue(payable(msg.sender), value);
    }
}

Exploit:
If I batch multiple payable subcall, they will all consider the msg.value. So calling
instance.multicall(["0x","0x","0x","0x","0x"], {value : 10**18 }) will credit me 5eth, despite only having deposited one.

Thus I could drain 4eth from the contract in a single call by doing :
instance.multicall(["0x","0x","0x","0x","0x", "0x853828b6" ], {value : 10**18 })

Beyond this simple example, any contract that considers msg.value, without comparing it to address(this).balance - interbookkeppingBalance is sensible ... this include almost all contracts with payable functions

Advice:
Add extensive warnings in the Multicall contract, or remove payability from it.

[moodysalem]

We don't actually use msg.value at all, just address(this).balance, but this is good to note. I added a comment to the method, and a unit test that demonstrates the behavior here

e652205

[WillSaveTime]

How I can use multicall function?