fix: recover sig failed in token recover portal (#451)

* fix: token recover contract recover approval sig * docs: update abi * fix: add generate option to tokenhub * fix: lint
bnb-chain · Dec 22, 2023 · 9a2d43e · 9a2d43e
1 parent 710d0b7
commit 9a2d43e
Show file tree

Hide file tree

Showing 5 changed files with 55 additions and 32 deletions.
diff --git a/abi/tokenrecoverportal.abi b/abi/tokenrecoverportal.abi
@@ -11,7 +11,7 @@
     },
     {
       "inputs": [],
-      "name": "InvalidApproverSignature",
+      "name": "InvalidApprovalSignature",
       "type": "error"
     },
     {
@@ -168,12 +168,25 @@
           "type": "uint256"
         }
       ],
-      "name": "TokenRecoverRequestd",
+      "name": "TokenRecoverRequested",
       "type": "event"
     },
     {
       "inputs": [],
-      "name": "approverAddress",
+      "name": "SOURCE_CHAIN_ID",
+      "outputs": [
+        {
+          "internalType": "string",
+          "name": "",
+          "type": "string"
+        }
+      ],
+      "stateMutability": "view",
+      "type": "function"
+    },
+    {
+      "inputs": [],
+      "name": "approvalAddress",
       "outputs": [
         {
           "internalType": "address",
@@ -312,19 +325,6 @@
       "stateMutability": "nonpayable",
       "type": "function"
     },
-    {
-      "inputs": [],
-      "name": "sourceChainID",
-      "outputs": [
-        {
-          "internalType": "string",
-          "name": "",
-          "type": "string"
-        }
-      ],
-      "stateMutability": "view",
-      "type": "function"
-    },
     {
       "inputs": [
         {
@@ -343,4 +343,4 @@
       "stateMutability": "nonpayable",
       "type": "function"
     }
-]
+  ]
diff --git a/contracts/BC_fusion/TokenRecoverPortal.sol b/contracts/BC_fusion/TokenRecoverPortal.sol
@@ -164,7 +164,25 @@ contract TokenRecoverPortal is System, ReentrancyGuardUpgradeable {
         // Perform the approvalSignature recovery and ensure the recovered signer is the approval account
         bytes32 hash =
             keccak256(abi.encodePacked(SOURCE_CHAIN_ID, account, ownerSignature, leafHash, merkleRoot, buffer));
-        if (ECDSA.recover(hash, approvalSignature) != approvalAddress) revert InvalidApprovalSignature();
+
+        if (recover(approvalSignature, hash) != approvalAddress) revert InvalidApprovalSignature();
+    }
+
+    function recover(bytes memory sig, bytes32 hash) internal pure returns (address) {
+        // Ensure the signature length is correct
+        if (sig.length != 65) revert InvalidApprovalSignature();
+        bytes32 r;
+        bytes32 s;
+        uint8 v;
+        assembly {
+            r := mload(add(sig, 32))
+            s := mload(add(sig, 64))
+            v := byte(0, mload(add(sig, 96)))
+        }
+        if (v < 27) v += 27;
+        if (v < 27 || v > 28) revert InvalidApprovalSignature();
+        (address signer,) = ECDSA.tryRecover(hash, v, r, s);
+        return signer;
     }
 
     /**
@@ -182,11 +200,11 @@ contract TokenRecoverPortal is System, ReentrancyGuardUpgradeable {
         if (signature.length != 64) revert InvalidOwnerSignatureLength();
 
         // assemble input data
-        bytes memory input = new bytes(129);
-        Utils.bytesConcat(input, pubKey, 0, 33);
-        Utils.bytesConcat(input, signature, 33, 64);
-        Utils.bytesConcat(input, abi.encodePacked(messageHash), 97, 32);
-
+        bytes memory msgBz = new bytes(32);
+        assembly {
+            mstore(add(msgBz, 32), messageHash)
+        }
+        bytes memory input = bytes.concat(pubKey, signature, msgBz);
         bytes memory output = new bytes(20);
         /* solium-disable-next-line */
         assembly {
@@ -199,7 +217,7 @@ contract TokenRecoverPortal is System, ReentrancyGuardUpgradeable {
             // | recovered address  |
             // | 20 bytes |
             let len := mload(input)
-            if iszero(staticcall(not(0), 0x69, input, len, output, 20)) { revert(0, 0) }
+            if iszero(staticcall(not(0), 0x69, add(input, 0x20), len, add(output, 0x20), 20)) { revert(0, 0) }
         }
 
         // return the recovered address
@@ -241,8 +259,7 @@ contract TokenRecoverPortal is System, ReentrancyGuardUpgradeable {
         } else if (key.compareStrings("merkleRoot")) {
             if (merkleRootAlreadyInit) revert MerkleRootAlreadyInitiated();
             if (value.length != 32) revert InvalidValue(key, value);
-            bytes32 newMerkleRoot = 0;
-            Utils.bytesToBytes32(32, value, newMerkleRoot);
+            bytes32 newMerkleRoot = value.bytesToBytes32(32);
             if (newMerkleRoot == bytes32(0)) revert InvalidValue(key, value);
             merkleRoot = newMerkleRoot;
             merkleRootAlreadyInit = true;

diff --git a/contracts/BC_fusion/lib/Utils.sol b/contracts/BC_fusion/lib/Utils.sol
@@ -24,10 +24,9 @@ library Utils {
         }
     }
 
-    function bytesToBytes32(uint256 _offst, bytes memory _input, bytes32 _output) internal pure {
+    function bytesToBytes32(bytes memory _input, uint256 _offset) internal pure returns (bytes32 _output) {
         assembly {
-            mstore(_output, add(_input, _offst))
-            mstore(add(_output, 32), add(add(_input, _offst), 32))
+            _output := mload(add(_input, _offset))
         }
     }
 

diff --git a/package.json b/package.json
@@ -7,7 +7,7 @@
   "scripts": {
     "lint:check": "forge fmt ./contracts/BC_fusion --check",
     "lint:write": "forge fmt ./contracts/BC_fusion",
-    "generate:dev": "poetry run python -m scripts.generate dev --init-felony-slash-scope \"60\" --breathe-block-interval \"1 minutes\" --block-interval \"1 seconds\" --init-bc-consensus-addresses 'hex\"00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000003000000000000000000000000f39fd6e51aad88f6f4ce6ab8827279cfffb9226600000000000000000000000070997970c51812dc3a010c7d01b50e0d17dc79c80000000000000000000000003c44cdddb6a900fa2b585dd299e03d12fa4293bc\"' --init-bc-vote-addresses 'hex\"00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000003000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000c000000000000000000000000000000000000000000000000000000000000001200000000000000000000000000000000000000000000000000000000000000030b86b3146bdd2200b1dbdb1cea5e40d3451c028cbb4fb03b1826f7f2d82bee76bbd5cd68a74a16a7eceea093fd5826b9200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000003087ce273bb9b51fd69e50de7a8d9a99cfb3b1a5c6a7b85f6673d137a5a2ce7df3d6ee4e6d579a142d58b0606c4a7a1c27000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000030a33ac14980d85c0d154c5909ebf7a11d455f54beb4d5d0dc1d8b3670b9c4a6b6c450ee3d623ecc48026f09ed1f0b5c1200000000000000000000000000000000\"' --asset-protector \"0xdF87F0e2B8519Ea2DD4aBd8B639cdD628497eD25\" --unbond-period \"2 minutes\" --downtime-jail-time \"2 minutes\" --felony-jail-time \"3 minutes\" --init-voting-delay \"1 minutes / BLOCK_INTERVAL\" --init-voting-period \"2 minutes / BLOCK_INTERVAL\" --init-min-period-after-quorum \"uint64(1 minutes / BLOCK_INTERVAL)\" --governor-protector \"0xdF87F0e2B8519Ea2DD4aBd8B639cdD628497eD25\" --init-minimal-delay \"1 minutes\""
+    "generate:dev": "poetry run python -m scripts.generate dev --init-felony-slash-scope \"60\" --breathe-block-interval \"1 minutes\" --block-interval \"3 seconds\" --init-bc-consensus-addresses 'hex\"00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000003000000000000000000000000f39fd6e51aad88f6f4ce6ab8827279cfffb9226600000000000000000000000070997970c51812dc3a010c7d01b50e0d17dc79c80000000000000000000000003c44cdddb6a900fa2b585dd299e03d12fa4293bc\"' --init-bc-vote-addresses 'hex\"00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000003000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000c000000000000000000000000000000000000000000000000000000000000001200000000000000000000000000000000000000000000000000000000000000030b86b3146bdd2200b1dbdb1cea5e40d3451c028cbb4fb03b1826f7f2d82bee76bbd5cd68a74a16a7eceea093fd5826b9200000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000003087ce273bb9b51fd69e50de7a8d9a99cfb3b1a5c6a7b85f6673d137a5a2ce7df3d6ee4e6d579a142d58b0606c4a7a1c27000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000030a33ac14980d85c0d154c5909ebf7a11d455f54beb4d5d0dc1d8b3670b9c4a6b6c450ee3d623ecc48026f09ed1f0b5c1200000000000000000000000000000000\"' --asset-protector \"0xdF87F0e2B8519Ea2DD4aBd8B639cdD628497eD25\" --unbond-period \"2 minutes\" --downtime-jail-time \"2 minutes\" --felony-jail-time \"3 minutes\" --init-voting-delay \"1 minutes / BLOCK_INTERVAL\" --init-voting-period \"2 minutes / BLOCK_INTERVAL\" --init-min-period-after-quorum \"uint64(1 minutes / BLOCK_INTERVAL)\" --governor-protector \"0xdF87F0e2B8519Ea2DD4aBd8B639cdD628497eD25\" --init-minimal-delay \"1 minutes\" --lock-period-for-token-recover \"1 minutes\""
   },
   "dependencies": {
     "@openzeppelin/contracts": "^4.9.3",

diff --git a/scripts/generate.py b/scripts/generate.py
@@ -209,7 +209,7 @@ def generate_tendermint_light_client(init_consensus_state_bytes, init_reward_for
     )
 
 
-def generate_token_hub(max_gas_for_transfer_bnb, max_gas_for_calling_bep20, reward_upper_limit, init_minimum_relay_fee):
+def generate_token_hub(max_gas_for_transfer_bnb, max_gas_for_calling_bep20, reward_upper_limit, init_minimum_relay_fee, lock_period_for_token_recover):
     contract = "TokenHub.sol"
     backup_file(
         os.path.join(work_dir, "contracts", contract), os.path.join(work_dir, "contracts", contract[:-4] + ".bak")
@@ -219,6 +219,7 @@ def generate_token_hub(max_gas_for_transfer_bnb, max_gas_for_calling_bep20, rewa
     replace_parameter(contract, "uint256 constant public MAX_GAS_FOR_CALLING_BEP20", f"{max_gas_for_calling_bep20}")
     replace_parameter(contract, "uint256 constant public REWARD_UPPER_LIMIT", f"{reward_upper_limit}")
     replace_parameter(contract, "uint256 constant public INIT_MINIMUM_RELAY_FEE", f"{init_minimum_relay_fee}")
+    replace_parameter(contract, "uint256 constant public LOCK_PERIOD_FOR_TOKEN_RECOVER", f"{lock_period_for_token_recover}")
 
 
 def generate_token_recover_portal(source_chain_id):
@@ -368,7 +369,12 @@ def dev(
     init_min_period_after_quorum: Annotated[
         str, typer.Option(help="INIT_MIN_PERIOD_AFTER_QUORUM of BSCGovernor")] = "uint64(1 days / BLOCK_INTERVAL)",
     governor_protector: Annotated[str, typer.Option(help="governorProtector of BSCGovernor")] = "address(0xdEaD)",
-    init_minimal_delay: Annotated[str, typer.Option(help="INIT_MINIMAL_DELAY of BSCTimelock")] = "24 hours"
+    init_minimal_delay: Annotated[str, typer.Option(help="INIT_MINIMAL_DELAY of BSCTimelock")] = "24 hours",
+    max_gas_for_transfer_bnb: Annotated[str, typer.Option(help="MAX_GAS_FOR_TRANSFER_BNB of TokenHub")]  = "10000",
+    max_gas_for_calling_bep20: Annotated[str, typer.Option(help="MAX_GAS_FOR_CALLING_BEP20 of TokenHub")]  = "50000",
+    reward_upper_limit: Annotated[str, typer.Option(help="REWARD_UPPER_LIMIT of TokenHub")]  = "1e18",
+    init_minimum_relay_fee: Annotated[str, typer.Option(help="INIT_MINIMUM_RELAY_FEE of TokenHub")]  = "2e15",
+    lock_period_for_token_recover: Annotated[str, typer.Option(help="LOCK_PERIOD_FOR_TOKEN_RECOVER of TokenHub")] = "7 days"
 ):
     global network, chain_id, hex_chain_id
     network = "dev"
@@ -406,6 +412,7 @@ def dev(
         block_interval, init_voting_delay, init_voting_period, init_min_period_after_quorum, governor_protector
     )
     generate_timelock(init_minimal_delay)
+    generate_token_hub(max_gas_for_transfer_bnb, max_gas_for_calling_bep20, reward_upper_limit, init_minimum_relay_fee, lock_period_for_token_recover)
 
     generate_genesis()
     print("Generate genesis of dev environment successfully")