🔒 Remove the multicall_value_self Function (#167)

pcaversaccio · Oct 13, 2023 · 55af6d8 · 55af6d8
1 parent 5f13de7
commit 55af6d8
Show file tree

Hide file tree

Showing 8 changed files with 81 additions and 342 deletions.
diff --git a/.gas-snapshot b/.gas-snapshot
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,11 @@
 # 🕓 Changelog
 
-## [`0.0.4`](https://github.com/pcaversaccio/snekmate/releases/tag/v0.0.4) (Unreleased)
+## [`0.0.4`](https://github.com/pcaversaccio/snekmate/releases/tag/v0.0.4) (13-10-2023)
+
+### 🔒 Security Fixes
+
+- **Utility Functions**
+  - [`Multicall`](https://github.com/pcaversaccio/snekmate/blob/v0.0.4/src/utils/Multicall.vy): Remove the `multicall_value_self` function as the `msg.value` should not be trusted. ([#167](https://github.com/pcaversaccio/snekmate/pull/167))
 
 ### 👀 Full Changelog
 

diff --git a/lib/openzeppelin-contracts b/lib/openzeppelin-contracts
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "snekmate",
-  "version": "0.0.4-rc.2",
+  "version": "0.0.4",
   "description": "State-of-the-art, highly opinionated, hyper-optimised, and secure 🐍Vyper smart contract building blocks.",
   "author": "Pascal Marco Caversaccio <[email protected]>",
   "license": "AGPL-3.0-only",

diff --git a/pyproject.toml b/pyproject.toml
@@ -1,6 +1,6 @@
 [project]
 name = "snekmate"
-version = "0.0.4rc2"
+version = "0.0.4"
 description = "State-of-the-art, highly opinionated, hyper-optimised, and secure 🐍Vyper smart contract building blocks."
 readme = {file = "README.md", content-type = "text/markdown"}
 requires-python = ">=3.10"

diff --git a/src/utils/Multicall.vy b/src/utils/Multicall.vy
@@ -12,7 +12,7 @@
         https://github.com/pcaversaccio/snekmate/discussions/82.
         The implementation is inspired by Matt Solomon's implementation here:
         https://github.com/mds1/multicall/blob/main/src/Multicall3.sol.
-@custom:security Make sure you understand how `msg.sender` works in `CALL` vs
+@custom:security Make sure you understand how `msg.sender` works in `CALL` vs.
                  `DELEGATECALL` to the multicall contract, as well as the risks
                  of using `msg.value` in a multicall. To learn more about the latter, see:
                  - https://github.com/runtimeverification/verified-smart-contracts/wiki/List-of-Security-Vulnerabilities#payable-multicall,
@@ -42,14 +42,6 @@ struct BatchSelf:
     call_data: Bytes[max_value(uint16)]
 
 
-# @dev Batch struct for `payable` function calls using this contract
-# as destination address. 
-struct BatchValueSelf:
-    allow_failure: bool
-    value: uint256
-    call_data: Bytes[max_value(uint16)]
-
-
 # @dev Result struct for function call results.
 struct Result:
     success: bool
@@ -164,47 +156,6 @@ def multicall_self(data: DynArray[BatchSelf, max_value(uint8)]) -> DynArray[Resu
     return results
 
 
-@external
-@payable
-def multicall_value_self(data: DynArray[BatchValueSelf, max_value(uint8)]) -> DynArray[Result, max_value(uint8)]:
-    """
-    @dev Aggregates function calls with a `msg.value` using
-         `DELEGATECALL`, ensuring that each function returns
-         successfully if required. Since this function uses
-         `DELEGATECALL`, the `msg.sender` remains the same
-         account that invoked the function `multicall_value_self`
-         in the first place.
-    @notice Developers can include this function in their own
-            contract so that users can submit multiple function
-            calls in one transaction. Since the `msg.sender` is
-            preserved, it's equivalent to sending multiple transactions
-            from an EOA (externally-owned account, i.e. non-contract account).
-
-            Furthermore, it is important to note that an external
-            call via `raw_call` does not perform an external code
-            size check on the target address.
-    @param data The array of `BatchValueSelf` structs.
-    @return DynArray The array of `Result` structs.
-    """
-    value_accumulator: uint256 = empty(uint256)
-    results: DynArray[Result, max_value(uint8)] = []
-    return_data: Bytes[max_value(uint8)] = b""
-    success: bool = empty(bool)
-    for batch in data:
-        msg_value: uint256 = batch.value
-        value_accumulator = unsafe_add(value_accumulator, msg_value)
-        if (batch.allow_failure == False):
-            return_data = raw_call(self, batch.call_data, max_outsize=255, value=msg_value, is_delegate_call=True)
-            success = True
-            results.append(Result({success: success, return_data: return_data}))
-        else:
-            success, return_data = \
-                raw_call(self, batch.call_data, max_outsize=255, value=msg_value, is_delegate_call=True, revert_on_failure=False)
-            results.append(Result({success: success, return_data: return_data}))
-    assert msg.value == value_accumulator, "Multicall: value mismatch"
-    return results
-
-
 @external
 @view
 def multistaticcall(data: DynArray[Batch, max_value(uint8)]) -> DynArray[Result, max_value(uint8)]:

diff --git a/test/utils/Multicall.t.sol b/test/utils/Multicall.t.sol
@@ -302,210 +302,6 @@ contract MulticallTest is Test {
         multicall.multicall_self(batchSelf);
     }
 
-    function testMulticallValueSelfSuccess() public {
-        IMulticall.BatchValue[] memory batchValue = new IMulticall.BatchValue[](
-            4
-        );
-        batchValue[0] = IMulticall.BatchValue(
-            mockCalleeAddr,
-            false,
-            0,
-            abi.encodeWithSignature("getBlockHash(uint256)", block.number)
-        );
-        batchValue[1] = IMulticall.BatchValue(
-            mockCalleeAddr,
-            false,
-            0,
-            abi.encodeWithSignature("store(uint256)", type(uint256).max)
-        );
-        batchValue[2] = IMulticall.BatchValue(
-            mockCalleeAddr,
-            true,
-            0,
-            abi.encodeWithSignature("thisMethodReverts()")
-        );
-        batchValue[3] = IMulticall.BatchValue(
-            mockCalleeAddr,
-            false,
-            1,
-            abi.encodeWithSignature("transferEther(address)", etherReceiverAddr)
-        );
-
-        IMulticall.Batch[] memory batch = new IMulticall.Batch[](2);
-        batch[0] = IMulticall.Batch(
-            mockCalleeAddr,
-            false,
-            abi.encodeWithSignature("getBlockHash(uint256)", block.number)
-        );
-        batch[1] = IMulticall.Batch(
-            mockCalleeAddr,
-            true,
-            abi.encodeWithSignature("thisMethodReverts()")
-        );
-
-        IMulticall.BatchValueSelf[]
-            memory batchValueSelf = new IMulticall.BatchValueSelf[](2);
-        batchValueSelf[0] = IMulticall.BatchValueSelf(
-            false,
-            1,
-            abi.encodeWithSignature(
-                "multicall_value((address,bool,uint256,bytes)[])",
-                batchValue
-            )
-        );
-        batchValueSelf[1] = IMulticall.BatchValueSelf(
-            true,
-            0,
-            abi.encodeWithSignature(
-                "multistaticcall((address,bool,bytes)[])",
-                batch
-            )
-        );
-
-        IMulticall.Result[] memory results = multicall.multicall_value_self{
-            value: 1
-        }(batchValueSelf);
-        assertTrue(results[0].success);
-        assertTrue(!results[1].success);
-        assertEq(etherReceiverAddr.balance, 1 wei);
-    }
-
-    function testMulticallValueSelfCase1() public {
-        IMulticall.BatchValue[] memory batchValue = new IMulticall.BatchValue[](
-            4
-        );
-        batchValue[0] = IMulticall.BatchValue(
-            mockCalleeAddr,
-            false,
-            0,
-            abi.encodeWithSignature("getBlockHash(uint256)", block.number)
-        );
-        batchValue[1] = IMulticall.BatchValue(
-            mockCalleeAddr,
-            false,
-            0,
-            abi.encodeWithSignature("store(uint256)", type(uint256).max)
-        );
-        /**
-         * @dev We don't allow for a failure.
-         */
-        batchValue[2] = IMulticall.BatchValue(
-            mockCalleeAddr,
-            false,
-            0,
-            abi.encodeWithSignature("thisMethodReverts()")
-        );
-        batchValue[3] = IMulticall.BatchValue(
-            mockCalleeAddr,
-            false,
-            1,
-            abi.encodeWithSignature("transferEther(address)", etherReceiverAddr)
-        );
-
-        IMulticall.Batch[] memory batch = new IMulticall.Batch[](2);
-        batch[0] = IMulticall.Batch(
-            mockCalleeAddr,
-            false,
-            abi.encodeWithSignature("getBlockHash(uint256)", block.number)
-        );
-        batch[1] = IMulticall.Batch(
-            mockCalleeAddr,
-            true,
-            abi.encodeWithSignature("thisMethodReverts()")
-        );
-
-        IMulticall.BatchValueSelf[]
-            memory batchValueSelf = new IMulticall.BatchValueSelf[](2);
-        batchValueSelf[0] = IMulticall.BatchValueSelf(
-            false,
-            1,
-            abi.encodeWithSignature(
-                "multicall_value((address,bool,uint256,bytes)[])",
-                batchValue
-            )
-        );
-        batchValueSelf[1] = IMulticall.BatchValueSelf(
-            true,
-            0,
-            abi.encodeWithSignature(
-                "multistaticcall((address,bool,bytes)[])",
-                batch
-            )
-        );
-
-        vm.expectRevert(
-            abi.encodeWithSelector(Reverted.selector, mockCalleeAddr)
-        );
-        multicall.multicall_value_self{value: 1}(batchValueSelf);
-    }
-
-    function testMulticallValueSelfCase2() public {
-        IMulticall.BatchValue[] memory batchValue = new IMulticall.BatchValue[](
-            4
-        );
-        batchValue[0] = IMulticall.BatchValue(
-            mockCalleeAddr,
-            false,
-            0,
-            abi.encodeWithSignature("getBlockHash(uint256)", block.number)
-        );
-        batchValue[1] = IMulticall.BatchValue(
-            mockCalleeAddr,
-            false,
-            0,
-            abi.encodeWithSignature("store(uint256)", type(uint256).max)
-        );
-        batchValue[2] = IMulticall.BatchValue(
-            mockCalleeAddr,
-            true,
-            0,
-            abi.encodeWithSignature("thisMethodReverts()")
-        );
-        batchValue[3] = IMulticall.BatchValue(
-            mockCalleeAddr,
-            false,
-            1,
-            abi.encodeWithSignature("transferEther(address)", etherReceiverAddr)
-        );
-
-        IMulticall.Batch[] memory batch = new IMulticall.Batch[](2);
-        batch[0] = IMulticall.Batch(
-            mockCalleeAddr,
-            false,
-            abi.encodeWithSignature("getBlockHash(uint256)", block.number)
-        );
-        batch[1] = IMulticall.Batch(
-            mockCalleeAddr,
-            true,
-            abi.encodeWithSignature("thisMethodReverts()")
-        );
-
-        IMulticall.BatchValueSelf[]
-            memory batchValueSelf = new IMulticall.BatchValueSelf[](2);
-        batchValueSelf[0] = IMulticall.BatchValueSelf(
-            false,
-            1,
-            abi.encodeWithSignature(
-                "multicall_value((address,bool,uint256,bytes)[])",
-                batchValue
-            )
-        );
-        batchValueSelf[1] = IMulticall.BatchValueSelf(
-            true,
-            0,
-            abi.encodeWithSignature(
-                "multistaticcall((address,bool,bytes)[])",
-                batch
-            )
-        );
-
-        vm.expectRevert(bytes("Multicall: value mismatch"));
-        /**
-         * @dev We send too much `msg.value`.
-         */
-        multicall.multicall_value_self{value: 2}(batchValueSelf);
-    }
-
     function testMultistaticcallSuccess() public {
         IMulticall.Batch[] memory batch = new IMulticall.Batch[](2);
         batch[0] = IMulticall.Batch(

diff --git a/test/utils/interfaces/IMulticall.sol b/test/utils/interfaces/IMulticall.sol
@@ -20,12 +20,6 @@ interface IMulticall {
         bytes callData;
     }
 
-    struct BatchValueSelf {
-        bool allowFailure;
-        uint256 value;
-        bytes callData;
-    }
-
     struct Result {
         bool success;
         bytes returnData;
@@ -43,10 +37,6 @@ interface IMulticall {
         BatchSelf[] calldata batchSelf
     ) external returns (Result[] memory results);
 
-    function multicall_value_self(
-        BatchValueSelf[] calldata batchValueSelf
-    ) external payable returns (Result[] memory results);
-
     function multistaticcall(
         Batch[] calldata batch
     ) external pure returns (Result[] memory results);