feat: limit permissionless generic call gas usage (#200)

sygmaprotocol · Oct 2, 2023 · d7823d7 · d7823d7
1 parent 4463bcb
commit d7823d7
Show file tree

Hide file tree

Showing 5 changed files with 103 additions and 5 deletions.
diff --git a/contracts/handlers/PermissionlessGenericHandler.sol b/contracts/handlers/PermissionlessGenericHandler.sol
@@ -10,6 +10,8 @@ import "../interfaces/IHandler.sol";
     @notice This contract is intended to be used with the Bridge contract.
  */
 contract PermissionlessGenericHandler is IHandler {
+    uint256 public constant MAX_FEE = 1000000;
+
     address public immutable _bridgeAddress;
 
     modifier onlyBridge() {
@@ -87,16 +89,19 @@ contract PermissionlessGenericHandler is IHandler {
     function deposit(bytes32 resourceID, address depositor, bytes calldata data) external view returns (bytes memory) {
         require(data.length >= 76, "Incorrect data length"); // 32 + 2 + 1 + 1 + 20 + 20
 
+        uint256        maxFee;
         uint16         lenExecuteFuncSignature;
         uint8          lenExecuteContractAddress;
         uint8          lenExecutionDataDepositor;
         address        executionDataDepositor;
 
+        maxFee                            = uint256(bytes32(data[:32]));
         lenExecuteFuncSignature           = uint16(bytes2(data[32:34]));
         lenExecuteContractAddress         = uint8(bytes1(data[34 + lenExecuteFuncSignature:35 + lenExecuteFuncSignature]));
         lenExecutionDataDepositor         = uint8(bytes1(data[35 + lenExecuteFuncSignature + lenExecuteContractAddress:36 + lenExecuteFuncSignature + lenExecuteContractAddress]));
         executionDataDepositor            = address(uint160(bytes20(data[36 + lenExecuteFuncSignature + lenExecuteContractAddress:36 + lenExecuteFuncSignature + lenExecuteContractAddress + lenExecutionDataDepositor])));
 
+        require(maxFee < MAX_FEE, 'requested fee too large');
         require(depositor == executionDataDepositor, 'incorrect depositor in deposit data');
     }
 
@@ -143,6 +148,7 @@ contract PermissionlessGenericHandler is IHandler {
           executeFuncSignature(address executionDataDepositor, uint[] uintArray, address addr)
      */
     function executeProposal(bytes32 resourceID, bytes calldata data) external onlyBridge returns (bytes memory) {
+        uint256        maxFee;
         uint16         lenExecuteFuncSignature;
         bytes4         executeFuncSignature;
         uint8          lenExecuteContractAddress;
@@ -151,6 +157,7 @@ contract PermissionlessGenericHandler is IHandler {
         address        executionDataDepositor;
         bytes   memory executionData;
 
+        maxFee                            = uint256(bytes32(data[0:32]));
         lenExecuteFuncSignature           = uint16(bytes2(data[32:34]));
         executeFuncSignature              = bytes4(data[34:34 + lenExecuteFuncSignature]);
         lenExecuteContractAddress         = uint8(bytes1(data[34 + lenExecuteFuncSignature:35 + lenExecuteFuncSignature]));
@@ -160,7 +167,7 @@ contract PermissionlessGenericHandler is IHandler {
         executionData                     = bytes(data[36 + lenExecuteFuncSignature + lenExecuteContractAddress + lenExecutionDataDepositor:]);
 
         bytes memory callData = abi.encodePacked(executeFuncSignature, abi.encode(executionDataDepositor), executionData);
-        (bool success, bytes memory returndata) = executeContractAddress.call(callData);
+        (bool success, bytes memory returndata) = executeContractAddress.call{gas: maxFee}(callData);
         return abi.encode(success, returndata);
     }
 }
diff --git a/test/handlers/fee/dynamic/calculateFeeGenericEVM.js b/test/handlers/fee/dynamic/calculateFeeGenericEVM.js
@@ -18,7 +18,7 @@ contract("DynamicGenericFeeHandlerEVM - [calculateFee]", async (accounts) => {
   const sender = accounts[0];
   const depositorAddress = accounts[1];
   const emptySetResourceData = "0x";
-  const destinationMaxFee = 2000000;
+  const destinationMaxFee = 900000;
   const msgGasLimit = 2300000;
   const ter = 0; // Not used
   const feeDataAmount = 0; // Not used

diff --git a/test/handlers/fee/dynamic/collectFeeGenericEVM.js b/test/handlers/fee/dynamic/collectFeeGenericEVM.js
@@ -20,7 +20,7 @@ contract("DynamicGenericFeeHandlerEVM - [collectFee]", async (accounts) => {
   const depositorAddress = accounts[1];
 
   const emptySetResourceData = "0x";
-  const destinationMaxFee = 2000000;
+  const destinationMaxFee = 900000;
   const msgGasLimit = 2300000;
   const hashOfTestStore = Ethers.utils.keccak256("0xc0ffee");
   const fee = Ethers.utils.parseEther("0.000036777");

diff --git a/test/handlers/generic/permissionlessDeposit.js b/test/handlers/generic/permissionlessDeposit.js
@@ -20,7 +20,7 @@ contract("PermissionlessGenericHandler - [deposit]", async (accounts) => {
   const depositorAddress = accounts[1];
 
   const feeData = "0x";
-  const destinationMaxFee = 2000000;
+  const destinationMaxFee = 900000;
   const hashOfTestStore = Ethers.utils.keccak256("0xc0ffee");
   const emptySetResourceData = "0x";
 
@@ -152,4 +152,27 @@ contract("PermissionlessGenericHandler - [deposit]", async (accounts) => {
       "incorrect depositor in deposit data"
     );
   });
+
+
+  it("should revert if max fee exceeds 1000000", async () => {
+    const invalidMaxFee = 1000001;
+    const invalidDepositData = Helpers.createPermissionlessGenericDepositData(
+      depositFunctionSignature,
+      TestStoreInstance.address,
+      invalidMaxFee ,
+      depositorAddress,
+      hashOfTestStore
+    );
+
+    await TruffleAssert.reverts(
+      BridgeInstance.deposit(
+        destinationDomainID,
+        resourceID,
+        invalidDepositData,
+        feeData,
+        {from: depositorAddress}
+      ),
+      "requested fee too large"
+    );
+  });
 });
diff --git a/test/handlers/generic/permissionlessExecuteProposal.js b/test/handlers/generic/permissionlessExecuteProposal.js
@@ -24,7 +24,7 @@ contract(
     const invalidExecutionContractAddress = accounts[4];
 
     const feeData = "0x";
-    const destinationMaxFee = 2000000;
+    const destinationMaxFee = 900000;
     const hashOfTestStore = Ethers.utils.keccak256("0xc0ffee");
     const handlerResponseLength = 64;
     const contractCallReturndata = Ethers.constants.HashZero;
@@ -226,6 +226,74 @@ contract(
       );
     });
 
+    it("ProposalExecution should be emitted even if gas specified too small", async () => {
+      const num = 6;
+      const addresses = [BridgeInstance.address, TestStoreInstance.address];
+      const message = Ethers.utils.hexlify(Ethers.utils.toUtf8Bytes("message"));
+      const executionData = Helpers.abiEncode(["uint", "address[]", "bytes"], [num, addresses, message]);
+
+      // If the target function accepts (address depositor, bytes executionData)
+      // then this helper can be used
+      const preparedExecutionData = await TestDepositInstance.prepareDepositData(executionData);
+      const depositFunctionSignature = Helpers.getFunctionSignature(
+        TestDepositInstance,
+        "executePacked"
+      );
+      const tooSmallGas = 500;
+      const depositData = Helpers.createPermissionlessGenericDepositData(
+        depositFunctionSignature,
+        TestDepositInstance.address,
+        tooSmallGas,
+        depositorAddress,
+        preparedExecutionData
+      );
+      const proposal = {
+        originDomainID: originDomainID,
+        depositNonce: expectedDepositNonce,
+        data: depositData,
+        resourceID: resourceID,
+      };
+      const proposalSignedData = await Helpers.signTypedProposal(
+        BridgeInstance.address,
+        [proposal]
+      );
+
+      // relayer1 executes the proposal
+      const executeTx = await BridgeInstance.executeProposal(
+        proposal,
+        proposalSignedData,
+        {from: relayer1Address}
+      );
+      // check that ProposalExecution event is emitted
+      TruffleAssert.eventEmitted(executeTx, "ProposalExecution", (event) => {
+        return (
+          event.originDomainID.toNumber() === originDomainID &&
+          event.depositNonce.toNumber() === expectedDepositNonce 
+        );
+      });
+
+      // check that deposit nonce isn't unmarked as used in bitmap
+      assert.isTrue(
+        await BridgeInstance.isProposalExecuted(
+          originDomainID,
+          expectedDepositNonce
+        )
+      );
+
+      const internalTx = await TruffleAssert.createTransactionResult(
+        TestDepositInstance,
+        executeTx.tx
+      );
+      TruffleAssert.eventNotEmitted(internalTx, "TestExecute", (event) => {
+        return (
+          event.depositor === depositorAddress &&
+          event.num.toNumber() === num &&
+          event.addr === TestStoreInstance.address &&
+          event.message === message
+        );
+      });
+    });
+
     it("call with packed depositData should be successful", async () => {
       const num = 5;
       const addresses = [BridgeInstance.address, TestStoreInstance.address];