feat: prevent on revert spoofing backport (#362)

zeta-chain · Sep 25, 2024 · a144b7a · a144b7a
1 parent 292d235
commit a144b7a
Show file tree

Hide file tree

Showing 32 changed files with 109 additions and 37 deletions.
diff --git a/.github/workflows/generated-files_v2.yaml b/.github/workflows/generated-files_v2.yaml
@@ -8,7 +8,7 @@ on:
       - "v2/**"
   pull_request:
     branches:
-      - "*"
+      - "**"
     paths:
       - "v2/**"
     types:

diff --git a/.github/workflows/lint_v2.yaml b/.github/workflows/lint_v2.yaml
@@ -8,7 +8,7 @@ on:
       - 'v2/**'
   pull_request:
     branches:
-      - "*"
+      - "**"
     paths:
       - 'v2/**'
     types:

diff --git a/.github/workflows/slither_v2.yaml b/.github/workflows/slither_v2.yaml
@@ -8,7 +8,7 @@ on:
       - 'v2/**'
   pull_request:
     branches:
-      - "*"
+      - "**"
     paths:
       - 'v2/**'
     types:

diff --git a/.github/workflows/test_v2.yaml b/.github/workflows/test_v2.yaml
@@ -8,7 +8,7 @@ on:
       - 'v2/**'
   pull_request:
     branches:
-      - "*"
+      - "**"
     paths:
       - 'v2/**'
     types:

diff --git a/v2/contracts/evm/GatewayEVM.sol b/v2/contracts/evm/GatewayEVM.sol
@@ -76,6 +76,7 @@ contract GatewayEVM is
     /// @param data Calldata to pass to the call.
     /// @return The result of the call.
     function _execute(address destination, bytes calldata data) internal returns (bytes memory) {
+        revertIfCallingOnRevert(data);
         (bool success, bytes memory result) = destination.call{ value: msg.value }(data);
         if (!success) revert ExecutionFailed();
 
@@ -385,4 +386,18 @@ contract GatewayEVM is
             IERC20(token).safeTransfer(custody, amount);
         }
     }
+
+    // @dev prevent spoofing onRevert functions
+    function revertIfCallingOnRevert(bytes calldata data) private pure {
+        if (data.length >= 4) {
+            bytes4 functionSelector;
+            assembly {
+                functionSelector := calldataload(data.offset)
+            }
+
+            if (functionSelector == Revertable.onRevert.selector) {
+                revert NotAllowedToCallOnRevert();
+            }
+        }
+    }
 }
diff --git a/v2/contracts/evm/interfaces/IGatewayEVM.sol b/v2/contracts/evm/interfaces/IGatewayEVM.sol
@@ -80,6 +80,9 @@ interface IGatewayEVMErrors {
 
     /// @notice Error when trying to transfer not whitelisted token to custody.
     error NotWhitelistedInCustody();
+
+    /// @notice Error when trying to call onRevert method using arbitrary call.
+    error NotAllowedToCallOnRevert();
 }
 
 /// @title IGatewayEVM

diff --git a/v2/pkg/erc20custody.sol/erc20custody.go b/v2/pkg/erc20custody.sol/erc20custody.go
diff --git a/v2/pkg/erc20custody.t.sol/erc20custodytest.go b/v2/pkg/erc20custody.t.sol/erc20custodytest.go
diff --git a/v2/pkg/erc20custodyechidnatest.sol/erc20custodyechidnatest.go b/v2/pkg/erc20custodyechidnatest.sol/erc20custodyechidnatest.go
diff --git a/v2/pkg/gatewayevm.sol/gatewayevm.go b/v2/pkg/gatewayevm.sol/gatewayevm.go
diff --git a/v2/pkg/gatewayevm.t.sol/gatewayevminboundtest.go b/v2/pkg/gatewayevm.t.sol/gatewayevminboundtest.go
diff --git a/v2/pkg/gatewayevm.t.sol/gatewayevmtest.go b/v2/pkg/gatewayevm.t.sol/gatewayevmtest.go
diff --git a/v2/pkg/gatewayevmechidnatest.sol/gatewayevmechidnatest.go b/v2/pkg/gatewayevmechidnatest.sol/gatewayevmechidnatest.go
diff --git a/v2/pkg/gatewayevmupgrade.t.sol/gatewayevmuupsupgradetest.go b/v2/pkg/gatewayevmupgrade.t.sol/gatewayevmuupsupgradetest.go
diff --git a/v2/pkg/gatewayevmupgradetest.sol/gatewayevmupgradetest.go b/v2/pkg/gatewayevmupgradetest.sol/gatewayevmupgradetest.go
diff --git a/v2/pkg/gatewayevmzevm.t.sol/gatewayevmzevmtest.go b/v2/pkg/gatewayevmzevm.t.sol/gatewayevmzevmtest.go
diff --git a/v2/pkg/igatewayevm.sol/igatewayevm.go b/v2/pkg/igatewayevm.sol/igatewayevm.go
diff --git a/v2/pkg/igatewayevm.sol/igatewayevmerrors.go b/v2/pkg/igatewayevm.sol/igatewayevmerrors.go
diff --git a/v2/pkg/zetaconnectornative.sol/zetaconnectornative.go b/v2/pkg/zetaconnectornative.sol/zetaconnectornative.go
diff --git a/v2/pkg/zetaconnectornative.t.sol/zetaconnectornativetest.go b/v2/pkg/zetaconnectornative.t.sol/zetaconnectornativetest.go
diff --git a/v2/pkg/zetaconnectornonnative.sol/zetaconnectornonnative.go b/v2/pkg/zetaconnectornonnative.sol/zetaconnectornonnative.go
diff --git a/v2/pkg/zetaconnectornonnative.t.sol/zetaconnectornonnativetest.go b/v2/pkg/zetaconnectornonnative.t.sol/zetaconnectornonnativetest.go
diff --git a/v2/test/GatewayEVM.t.sol b/v2/test/GatewayEVM.t.sol
@@ -182,6 +182,14 @@ contract GatewayEVMTest is Test, IGatewayEVMErrors, IGatewayEVMEvents, IReceiver
         gateway.execute(address(receiver), data);
     }
 
+    function testForwardCallToReceiveOnRevertFails() public {
+        bytes memory data = abi.encodeWithSignature("onRevert((address,uint64,bytes))");
+
+        vm.prank(tssAddress);
+        vm.expectRevert(NotAllowedToCallOnRevert.selector);
+        gateway.execute(address(receiver), data);
+    }
+
     function testExecuteFailsIfDestinationIsZeroAddress() public {
         bytes memory data = abi.encodeWithSignature("receiveNoParams()");
 

diff --git a/v2/types/factories/ERC20CustodyEchidnaTest__factory.ts b/v2/types/factories/ERC20CustodyEchidnaTest__factory.ts
diff --git a/v2/types/factories/ERC20Custody__factory.ts b/v2/types/factories/ERC20Custody__factory.ts
diff --git a/v2/types/factories/GatewayEVMEchidnaTest__factory.ts b/v2/types/factories/GatewayEVMEchidnaTest__factory.ts
diff --git a/v2/types/factories/GatewayEVMUpgradeTest__factory.ts b/v2/types/factories/GatewayEVMUpgradeTest__factory.ts
diff --git a/v2/types/factories/GatewayEVM__factory.ts b/v2/types/factories/GatewayEVM__factory.ts
diff --git a/v2/types/factories/IGatewayEVM.sol/IGatewayEVMErrors__factory.ts b/v2/types/factories/IGatewayEVM.sol/IGatewayEVMErrors__factory.ts
@@ -44,6 +44,11 @@ const _abi = [
     name: "InsufficientETHAmount",
     inputs: [],
   },
+  {
+    type: "error",
+    name: "NotAllowedToCallOnRevert",
+    inputs: [],
+  },
   {
     type: "error",
     name: "NotWhitelistedInCustody",

diff --git a/v2/types/factories/IGatewayEVM.sol/IGatewayEVM__factory.ts b/v2/types/factories/IGatewayEVM.sol/IGatewayEVM__factory.ts
@@ -684,6 +684,11 @@ const _abi = [
     name: "InsufficientETHAmount",
     inputs: [],
   },
+  {
+    type: "error",
+    name: "NotAllowedToCallOnRevert",
+    inputs: [],
+  },
   {
     type: "error",
     name: "NotWhitelistedInCustody",

diff --git a/v2/types/factories/ZetaConnectorNative__factory.ts b/v2/types/factories/ZetaConnectorNative__factory.ts
diff --git a/v2/types/factories/ZetaConnectorNonNative__factory.ts b/v2/types/factories/ZetaConnectorNonNative__factory.ts
-Original file line number
+Diff line change
@@ Expand Up / @@ -8,7 +8,7 @@ on: @@
           - "v2/**"
       pull_request:
         branches:
-          - "*"
+          - "**"
         paths:
           - "v2/**"
         types:
@@ Expand Down @@