chore: PermissionlessGenericHandler: unpack depositor address with cu…

…stom length (#161)

contracts/TestContracts.sol

@@ -171,3 +171,39 @@ contract ERC20PresetMinterPauserDecimals is ERC20PresetMinterPauser {
         return customDecimals;
     }
 }
+
+contract TestDeposit {
+    event TestExecute(address depositor, uint256 num, address addr, bytes message);
+
+    /**
+        This helper can be used to prepare execution data for Bridge.deposit() on the source chain
+        if PermissionlessGenericHandler is used 
+        and if the target function accepts (address depositor, bytes executionData).
+        The execution data (packed as bytes) will be packed together with depositorAddress 
+        in PermissionlessGenericHandler before execution on the target chain.
+        This function packs the bytes parameter together with a fake address and removes the address.
+        After repacking in the handler together with depositorAddress, the offsets will be correct.
+        Usage: pack all parameters as bytes, then use this function, then pack the result of this function
+        together with maxFee, executeFuncSignature etc and pass it to Bridge.deposit().
+    */
+    function prepareDepositData(bytes calldata executionData) view external returns (bytes memory) {
+        bytes memory encoded = abi.encode(address(0), executionData);
+        return this.slice(encoded, 32);
+    }
+
+    function slice(bytes calldata input, uint256 position) pure public returns (bytes memory) {
+        return input[position:];
+    }
+
+    function executePacked(address depositor, bytes calldata data) external {
+        uint256 num;
+        address[] memory addresses;
+        bytes memory message;
+        (num, addresses, message) = abi.decode(data, (uint256, address[], bytes));
+        emit TestExecute(depositor, num, addresses[1], message);
+    }
+
+    function executeUnpacked(address depositor, uint256 num, address[] memory addresses, bytes memory message) external {
+        emit TestExecute(depositor, num, addresses[1], message);
+    }
+}

contracts/handlers/PermissionlessGenericHandler.sol

@@ -55,9 +55,37 @@ contract PermissionlessGenericHandler is IHandler {
           len(executionDataDepositor):  uint8    bytes  35 + len(executeFuncSignature) + len(executeContractAddress)                                -  36 + len(executeFuncSignature) + len(executeContractAddress)
           executionDataDepositor:       bytes    bytes  36 + len(executeFuncSignature) + len(executeContractAddress)                                -  36 + len(executeFuncSignature) + len(executeContractAddress) + len(executionDataDepositor)
           executionData:                bytes    bytes  36 + len(executeFuncSignature) + len(executeContractAddress) + len(executionDataDepositor)  -  END
+
+          executionData is repacked together with executionDataDepositor address for using it in the target contract.
+          If executionData contains dynamic types then it is necessary to keep the offsets correct.
+          executionData should be encoded together with a 32-byte address and then passed as a parameter without that address.
+          If the target function accepts (address depositor, bytes executionData) 
+          then a function like the following one can be used:
+
+            function prepareDepositData(bytes calldata executionData) view external returns (bytes memory) {
+                bytes memory encoded = abi.encode(address(0), executionData);
+                return this.slice(encoded, 32);
+            }
+
+            function slice(bytes calldata input, uint256 position) pure public returns (bytes memory) {
+                return input[position:];
+            }
+          After this, the target contract will get the following:
+          executeFuncSignature(address executionDataDepositor, bytes executionData)
+
+          Another example: if the target function accepts (address depositor, uint[], address)
+          then a function like the following one can be used:
+            
+            function prepareDepositData(uint[] calldata uintArray, address addr) view external returns (bytes memory) {
+                bytes memory encoded = abi.encode(address(0), uintArray, addr);
+                return this.slice(encoded, 32);
+            }
+
+          After this, the target contract will get the following:
+          executeFuncSignature(address executionDataDepositor, uint[] uintArray, address addr)
      */
     function deposit(bytes32 resourceID, address depositor, bytes calldata data) external view returns (bytes memory) {
-        require(data.length > 81, "Incorrect data length");
+        require(data.length >= 76, "Incorrect data length"); // 32 + 2 + 1 + 1 + 20 + 20
 
         uint16         lenExecuteFuncSignature;
         uint8          lenExecuteContractAddress;
@@ -67,7 +95,7 @@ contract PermissionlessGenericHandler is IHandler {
         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            = abi.decode(data[36 + lenExecuteFuncSignature + lenExecuteContractAddress:36 + lenExecuteFuncSignature + lenExecuteContractAddress + lenExecutionDataDepositor], (address));
+        executionDataDepositor            = address(uint160(bytes20(data[36 + lenExecuteFuncSignature + lenExecuteContractAddress:36 + lenExecuteFuncSignature + lenExecuteContractAddress + lenExecutionDataDepositor])));
 
         require(depositor == executionDataDepositor, 'incorrect depositor in deposit data');
     }
@@ -82,22 +110,56 @@ contract PermissionlessGenericHandler is IHandler {
           len(executeContractAddress):        uint8    bytes  34 + len(executeFuncSignature)                                -  35 + len(executeFuncSignature)
           executeContractAddress              bytes    bytes  35 + len(executeFuncSignature)                                -  35 + len(executeFuncSignature) + len(executeContractAddress)
           len(executionDataDepositor):        uint8    bytes  35 + len(executeFuncSignature) + len(executeContractAddress)  -  36 + len(executeFuncSignature) + len(executeContractAddress)
-          executionDataDepositorWithData:     bytes    bytes  36 + len(executeFuncSignature) + len(executeContractAddress)  -  END
+          executionDataDepositor:             bytes    bytes  36 + len(executeFuncSignature) + len(executeContractAddress)                                -  36 + len(executeFuncSignature) + len(executeContractAddress) + len(executionDataDepositor)
+          executionData:                      bytes    bytes  36 + len(executeFuncSignature) + len(executeContractAddress) + len(executionDataDepositor)  -  END
+
+          executionData is repacked together with executionDataDepositor address for using it in the target contract.
+          If executionData contains dynamic types then it is necessary to keep the offsets correct.
+          executionData should be encoded together with a 32-byte address and then passed as a parameter without that address.
+          If the target function accepts (address depositor, bytes executionData) 
+          then a function like the following one can be used:
+
+            function prepareDepositData(bytes calldata executionData) view external returns (bytes memory) {
+                bytes memory encoded = abi.encode(address(0), executionData);
+                return this.slice(encoded, 32);
+            }
+
+            function slice(bytes calldata input, uint256 position) pure public returns (bytes memory) {
+                return input[position:];
+            }
+
+          After this, the target contract will get the following:
+          executeFuncSignature(address executionDataDepositor, bytes executionData)
+
+          Another example: if the target function accepts (address depositor, uint[], address)
+          then a function like the following one can be used:
+            
+            function prepareDepositData(uint[] calldata uintArray, address addr) view external returns (bytes memory) {
+                bytes memory encoded = abi.encode(address(0), uintArray, addr);
+                return this.slice(encoded, 32);
+            }
+
+          After this, the target contract will get the following:
+          executeFuncSignature(address executionDataDepositor, uint[] uintArray, address addr)
      */
     function executeProposal(bytes32 resourceID, bytes calldata data) external onlyBridge {
         uint16         lenExecuteFuncSignature;
         bytes4         executeFuncSignature;
         uint8          lenExecuteContractAddress;
         address        executeContractAddress;
-        bytes   memory executionDataDepositorWithData;
+        uint8          lenExecutionDataDepositor;
+        address        executionDataDepositor;
+        bytes   memory executionData;
 
         lenExecuteFuncSignature           = uint16(bytes2(data[32:34]));
         executeFuncSignature              = bytes4(data[34:34 + lenExecuteFuncSignature]);
         lenExecuteContractAddress         = uint8(bytes1(data[34 + lenExecuteFuncSignature:35 + lenExecuteFuncSignature]));
         executeContractAddress            = address(uint160(bytes20(data[35 + lenExecuteFuncSignature:35 + lenExecuteFuncSignature + lenExecuteContractAddress])));
-        executionDataDepositorWithData    = bytes(data[36 + lenExecuteFuncSignature + lenExecuteContractAddress:]);
+        lenExecutionDataDepositor         = uint8(bytes1(data[35 + lenExecuteFuncSignature + lenExecuteContractAddress:36 + lenExecuteFuncSignature + lenExecuteContractAddress]));
+        executionDataDepositor            = address(uint160(bytes20(data[36 + lenExecuteFuncSignature + lenExecuteContractAddress:36 + lenExecuteFuncSignature + lenExecuteContractAddress + lenExecutionDataDepositor])));
+        executionData                     = bytes(data[36 + lenExecuteFuncSignature + lenExecuteContractAddress + lenExecutionDataDepositor:]);
 
-        bytes memory callData = abi.encodePacked(executeFuncSignature, executionDataDepositorWithData);
+        bytes memory callData = abi.encodePacked(executeFuncSignature, abi.encode(executionDataDepositor), executionData);
         executeContractAddress.call(callData);
     }
 }

test/handlers/generic/permissionlessDeposit.js

@@ -117,7 +117,8 @@ contract("PermissionlessGenericHandler - [deposit]", async (accounts) => {
   });
 
   it("deposit data should be of required length", async () => {
-    const invalidDepositData = "0x" + "02a3d".repeat(31);
+    // Min length is 76 bytes
+    const invalidDepositData = "0x" + "aa".repeat(75);
 
     await TruffleAssert.reverts(
       BridgeInstance.deposit(

test/handlers/generic/permissionlessExecuteProposal.js

@@ -8,6 +8,7 @@ const Ethers = require("ethers");
 const Helpers = require("../../helpers");
 
 const TestStoreContract = artifacts.require("TestStore");
+const TestDepositContract = artifacts.require("TestDeposit");
 const PermissionlessGenericHandlerContract = artifacts.require(
   "PermissionlessGenericHandler"
 );
@@ -30,6 +31,7 @@ contract(
 
     let BridgeInstance;
     let TestStoreInstance;
+    let TestDepositInstance;
 
     let resourceID;
     let depositFunctionSignature;
@@ -46,6 +48,9 @@ contract(
         TestStoreContract.new().then(
           (instance) => (TestStoreInstance = instance)
         ),
+        TestDepositContract.new().then(
+          (instance) => (TestDepositInstance = instance)
+        ),
       ]);
 
       resourceID = Helpers.createResourceID(
@@ -215,5 +220,143 @@ contract(
         await TestStoreInstance._assetsStored.call(hashOfTestStore)
       );
     });
+
+    it("call with packed depositData should be successful", async () => {
+      const num = 5;
+      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 depositData = Helpers.createPermissionlessGenericDepositData(
+        depositFunctionSignature,
+        TestDepositInstance.address,
+        destinationMaxFee,
+        depositorAddress,
+        preparedExecutionData
+      );
+
+      const proposal = {
+        originDomainID: originDomainID,
+        depositNonce: expectedDepositNonce,
+        data: depositData,
+        resourceID: resourceID,
+      };
+      const proposalSignedData = await Helpers.signTypedProposal(
+        BridgeInstance.address,
+        [proposal]
+      );
+      await TruffleAssert.passes(
+        BridgeInstance.deposit(
+          originDomainID,
+          resourceID,
+          depositData,
+          feeData,
+          {from: depositorAddress}
+        )
+      );
+
+      // relayer1 executes the proposal
+      const executeTx = await BridgeInstance.executeProposal(proposal, proposalSignedData, {
+        from: relayer1Address,
+      });
+
+      const internalTx = await TruffleAssert.createTransactionResult(
+        TestDepositInstance,
+        executeTx.tx
+      );
+
+      // check that ProposalExecution event is emitted
+      TruffleAssert.eventEmitted(executeTx, "ProposalExecution", (event) => {
+        return (
+          event.originDomainID.toNumber() === originDomainID &&
+          event.depositNonce.toNumber() === expectedDepositNonce
+        );
+      });
+
+      TruffleAssert.eventEmitted(internalTx, "TestExecute", (event) => {
+        return (
+          event.depositor === depositorAddress &&
+          event.num.toNumber() === num &&
+          event.addr === TestStoreInstance.address &&
+          event.message === message
+        );
+      });
+    });
+
+    it("call with unpacked depositData should be successful", async () => {
+      const num = 5;
+      const addresses = [BridgeInstance.address, TestStoreInstance.address];
+      const message = Ethers.utils.hexlify(Ethers.utils.toUtf8Bytes("message"));
+
+      const executionData = Helpers.createPermissionlessGenericExecutionData(
+        ["uint", "address[]", "bytes"], [num, addresses, message]
+      );
+
+      const depositFunctionSignature = Helpers.getFunctionSignature(
+        TestDepositInstance,
+        "executeUnpacked"
+      );
+      const depositData = Helpers.createPermissionlessGenericDepositData(
+        depositFunctionSignature,
+        TestDepositInstance.address,
+        destinationMaxFee,
+        depositorAddress,
+        executionData
+      );
+
+      const proposal = {
+        originDomainID: originDomainID,
+        depositNonce: expectedDepositNonce,
+        data: depositData,
+        resourceID: resourceID,
+      };
+      const proposalSignedData = await Helpers.signTypedProposal(
+        BridgeInstance.address,
+        [proposal]
+      );
+      await TruffleAssert.passes(
+        BridgeInstance.deposit(
+          originDomainID,
+          resourceID,
+          depositData,
+          feeData,
+          {from: depositorAddress}
+        )
+      );
+
+      // relayer1 executes the proposal
+      const executeTx = await BridgeInstance.executeProposal(proposal, proposalSignedData, {
+        from: relayer1Address,
+      });
+
+      const internalTx = await TruffleAssert.createTransactionResult(
+        TestDepositInstance,
+        executeTx.tx
+      );
+
+      // check that ProposalExecution event is emitted
+      TruffleAssert.eventEmitted(executeTx, "ProposalExecution", (event) => {
+        return (
+          event.originDomainID.toNumber() === originDomainID &&
+          event.depositNonce.toNumber() === expectedDepositNonce
+        );
+      });
+
+      TruffleAssert.eventEmitted(internalTx, "TestExecute", (event) => {
+        return (
+          event.depositor === depositorAddress &&
+          event.num.toNumber() === num &&
+          event.addr === TestStoreInstance.address &&
+          event.message === message
+        );
+      });
+    });
   }
 );

test/helpers.js

@@ -150,8 +150,8 @@ const createPermissionlessGenericDepositData = (
     executeFunctionSignature.substr(2) + // bytes
     toHex(executeContractAddress.substr(2).length / 2, 1).substr(2) + // uint8
     executeContractAddress.substr(2) + // bytes
-    toHex(32, 1).substr(2) + // uint8
-    toHex(depositor, 32).substr(2) + // bytes32
+    toHex(depositor.substr(2).length / 2, 1).substr(2) + // uint8
+    depositor.substr(2) + // bytes
     executionData.substr(2)
   ) // bytes
     .toLowerCase();
@@ -343,6 +343,24 @@ const createDepositProposalDataFromHandlerResponse = (
 };
 
 
+// This helper can be used to prepare execution data for PermissionlessGenericHandler
+// The execution data will be packed together with depositorAddress before execution.
+// If the target function parameters include reference types then the offsets should be kept consistent.
+// This function packs the parameters together with a fake address and removes the address.
+// After repacking the data in the handler together with depositorAddress, the offsets will be correct.
+// Usage: use this function to prepare execution data,
+// then pack the result together with executeFunctionSignature, maxFee etc 
+// (using the createPermissionlessGenericDepositData() helper)
+// and then pass the data to Bridge.deposit().
+const createPermissionlessGenericExecutionData = (
+  types,
+  values
+) => {
+  types.unshift("address");
+  values.unshift(Ethers.constants.AddressZero);
+  return "0x" + abiEncode(types, values).substr(66);
+};
+
 module.exports = {
   advanceBlock,
   advanceTime,
@@ -373,4 +391,5 @@ module.exports = {
   signTypedProposal,
   mockSignTypedProposalWithInvalidChainID,
   createDepositProposalDataFromHandlerResponse,
+  createPermissionlessGenericExecutionData
 };