Signature verification Audit request (#16934)

1. update signature verification to revert with duplicate, and invalid signatures instead of skipping. 2. include verification of provided "Sui Decimals" in Add EVM Token message function.
MystenLabs · May 9, 2024 · 5d5c532 · 5d5c532
1 parent fefe4f7
commit 5d5c532
Show file tree

Hide file tree

Showing 9 changed files with 180 additions and 29 deletions.
diff --git a/bridge/evm/contracts/BridgeCommittee.sol b/bridge/evm/contracts/BridgeCommittee.sol
@@ -88,17 +88,13 @@ contract BridgeCommittee is IBridgeCommittee, CommitteeUpgradeable {
 
             (signer,,) = ECDSA.tryRecover(BridgeUtils.computeHash(message), v, r, s);
 
-            // skip if signer is block listed or has no stake
-            if (blocklist[signer] || committeeStake[signer] == 0) continue;
+            require(!blocklist[signer], "BridgeCommittee: Signer is blocklisted");
+            require(committeeStake[signer] > 0, "BridgeCommittee: Signer has no stake");
 
             uint8 index = committeeIndex[signer];
             uint256 mask = 1 << index;
-            if (bitmap & mask == 0) {
-                bitmap |= mask;
-            } else {
-                // skip if duplicate signature
-                continue;
-            }
+            require(bitmap & mask == 0, "BridgeCommittee: Duplicate signature provided");
+            bitmap |= mask;
 
             approvalStake += committeeStake[signer];
         }

diff --git a/bridge/evm/contracts/BridgeConfig.sol b/bridge/evm/contracts/BridgeConfig.sol
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.20;
 
+import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
 import "./utils/CommitteeUpgradeable.sol";
 import "./interfaces/IBridgeConfig.sol";
 
@@ -167,6 +168,9 @@ contract BridgeConfig is IBridgeConfig, CommitteeUpgradeable {
         require(suiDecimal > 0, "BridgeConfig: Invalid Sui decimal");
         require(tokenPrice > 0, "BridgeConfig: Invalid token price");
 
+        uint8 erc20Decimals = IERC20Metadata(tokenAddress).decimals();
+        require(erc20Decimals >= suiDecimal, "BridgeConfig: Invalid Sui decimal");
+
         supportedTokens[tokenID] = Token(tokenAddress, suiDecimal, native);
         tokenPrices[tokenID] = tokenPrice;
 

diff --git a/bridge/evm/foundry.toml b/bridge/evm/foundry.toml
@@ -1,7 +1,7 @@
 [profile.default]
 src = 'contracts'
 test = 'test'
-no_match_test = "testMock"
+no_match_test = "testSkip"
 out = 'out'
 libs = ['lib']
 solc = "0.8.20"

diff --git a/bridge/evm/script/deploy_bridge.s.sol b/bridge/evm/script/deploy_bridge.s.sol
@@ -169,7 +169,7 @@ contract DeployBridge is Script {
     }
 
     // used to ignore for forge coverage
-    function test() public {}
+    function testSkip() public {}
 }
 
 /// check the following for guidelines on updating deploy_configs and references:

diff --git a/bridge/evm/test/BridgeBaseTest.t.sol b/bridge/evm/test/BridgeBaseTest.t.sol
@@ -134,7 +134,7 @@ contract BridgeBaseTest is Test {
         limiter.transferOwnership(address(bridge));
     }
 
-    function testMock() public {}
+    function testSkip() public {}
 
     // Helper function to get the signature components from an address
     function getSignature(bytes32 digest, uint256 privateKey) public pure returns (bytes memory) {

diff --git a/bridge/evm/test/BridgeCommitteeTest.t.sol b/bridge/evm/test/BridgeCommitteeTest.t.sol
@@ -159,7 +159,7 @@ contract BridgeCommitteeTest is BridgeBaseTest {
         signatures[3] = getSignature(messageHash, committeeMemberPkC);
 
         // Call the verifySignatures function and expect it to revert
-        vm.expectRevert(bytes("BridgeCommittee: Insufficient stake amount"));
+        vm.expectRevert(bytes("BridgeCommittee: Duplicate signature provided"));
         committee.verifySignatures(signatures, message);
     }
 
@@ -271,8 +271,6 @@ contract BridgeCommitteeTest is BridgeBaseTest {
 
         assertTrue(committee.blocklist(committeeMemberA));
 
-        // verify CommitteeMemberA's signature is no longer valid
-        vm.expectRevert(bytes("BridgeCommittee: Insufficient stake amount"));
         // update message
         message.nonce = 1;
         // reconstruct signatures
@@ -282,6 +280,8 @@ contract BridgeCommitteeTest is BridgeBaseTest {
         signatures[1] = getSignature(messageHash, committeeMemberPkB);
         signatures[2] = getSignature(messageHash, committeeMemberPkC);
         signatures[3] = getSignature(messageHash, committeeMemberPkD);
+        // verify CommitteeMemberA's signature is no longer valid
+        vm.expectRevert(bytes("BridgeCommittee: Signer is blocklisted"));
         // re-verify signatures
         committee.verifySignatures(signatures, message);
     }
@@ -311,7 +311,7 @@ contract BridgeCommitteeTest is BridgeBaseTest {
         signatures[2] = getSignature(messageHash, committeeMemberPkC);
         signatures[3] = getSignature(messageHash, committeeMemberPkF);
 
-        vm.expectRevert(bytes("BridgeCommittee: Insufficient stake amount"));
+        vm.expectRevert(bytes("BridgeCommittee: Signer has no stake"));
         committee.verifySignatures(signatures, message);
     }
 
@@ -460,7 +460,7 @@ contract BridgeCommitteeTest is BridgeBaseTest {
         signatures[2] =
             hex"62b36dab0d2c10f74d84b5f9838435c396cca1f3c4939eb4df82d1c72430e7ec2a030a980a9514beaeda6dffdc5e177b7edbd18543979f488d8fd09dba753a5500";
 
-        vm.expectRevert(bytes("BridgeCommittee: Insufficient stake amount"));
+        vm.expectRevert(bytes("BridgeCommittee: Signer is blocklisted"));
         committee.verifySignatures(signatures, message);
 
         // use sig from a unblocklisted validator

diff --git a/bridge/evm/test/BridgeConfigTest.t.sol b/bridge/evm/test/BridgeConfigTest.t.sol
@@ -2,6 +2,7 @@
 pragma solidity ^0.8.20;
 
 import "./BridgeBaseTest.t.sol";
+import "./mocks/MockTokens.sol";
 
 contract BridgeConfigTest is BridgeBaseTest {
     function setUp() public {
@@ -41,14 +42,16 @@ contract BridgeConfigTest is BridgeBaseTest {
     }
 
     function testAddTokensWithSignatures() public {
+        MockUSDC _newToken = new MockUSDC();
+
         // Create update tokens payload
         bool _isNative = true;
         uint8 _numTokenIDs = 1;
         uint8 tokenID1 = 10;
         uint8 _numAddresses = 1;
-        address address1 = address(10101);
+        address address1 = address(_newToken);
         uint8 _numSuiDecimals = 1;
-        uint8 suiDecimal1 = 8;
+        uint8 suiDecimal1 = 6;
         uint8 _numPrices = 1;
         uint64 price1 = 100_000_0000;
 
@@ -64,8 +67,6 @@ contract BridgeConfigTest is BridgeBaseTest {
             price1
         );
 
-        console.logBytes(payload);
-
         // Create transfer message
         BridgeUtils.Message memory message = BridgeUtils.Message({
             messageType: BridgeUtils.ADD_EVM_TOKENS,
@@ -90,20 +91,170 @@ contract BridgeConfigTest is BridgeBaseTest {
         assertFalse(config.isTokenSupported(10));
         config.addTokensWithSignatures(signatures, message);
         assertTrue(config.isTokenSupported(10));
-        assertEq(config.tokenAddressOf(10), address(10101));
-        assertEq(config.tokenSuiDecimalOf(10), 8);
+        assertEq(config.tokenAddressOf(10), address1);
+        assertEq(config.tokenSuiDecimalOf(10), 6);
         assertEq(config.tokenPriceOf(10), 100_000_0000);
     }
 
+    function testAddTokensAddressFailure() public {
+        MockUSDC _newToken = new MockUSDC();
+
+        // Create update tokens payload
+        bool _isNative = true;
+        uint8 _numTokenIDs = 1;
+        uint8 tokenID1 = 10;
+        uint8 _numAddresses = 1;
+        address address1 = address(0);
+        uint8 _numSuiDecimals = 1;
+        uint8 suiDecimal1 = 6;
+        uint8 _numPrices = 1;
+        uint64 price1 = 100_000_00000000;
+
+        bytes memory payload = abi.encodePacked(
+            _isNative,
+            _numTokenIDs,
+            tokenID1,
+            _numAddresses,
+            address1,
+            _numSuiDecimals,
+            suiDecimal1,
+            _numPrices,
+            price1
+        );
+
+        // Create Add evm token message
+        BridgeUtils.Message memory message = BridgeUtils.Message({
+            messageType: BridgeUtils.ADD_EVM_TOKENS,
+            version: 1,
+            nonce: 0,
+            chainID: 1,
+            payload: payload
+        });
+
+        bytes memory encodedMessage = BridgeUtils.encodeMessage(message);
+
+        bytes32 messageHash = keccak256(encodedMessage);
+
+        bytes[] memory signatures = new bytes[](4);
+
+        signatures[0] = getSignature(messageHash, committeeMemberPkA);
+        signatures[1] = getSignature(messageHash, committeeMemberPkB);
+        signatures[2] = getSignature(messageHash, committeeMemberPkC);
+        signatures[3] = getSignature(messageHash, committeeMemberPkD);
+
+        // address should fail because the address supplied in the message is 0
+        vm.expectRevert(bytes("BridgeConfig: Invalid token address"));
+        config.addTokensWithSignatures(signatures, message);
+    }
+
+    function testAddTokensSuiDecimalFailure() public {
+        MockUSDC _newToken = new MockUSDC();
+
+        // Create add tokens payload
+        bool _isNative = true;
+        uint8 _numTokenIDs = 1;
+        uint8 tokenID1 = 10;
+        uint8 _numAddresses = 1;
+        address address1 = address(_newToken);
+        uint8 _numSuiDecimals = 1;
+        uint8 suiDecimal1 = 10;
+        uint8 _numPrices = 1;
+        uint64 price1 = 100_000_00000000;
+
+        bytes memory payload = abi.encodePacked(
+            _isNative,
+            _numTokenIDs,
+            tokenID1,
+            _numAddresses,
+            address1,
+            _numSuiDecimals,
+            suiDecimal1,
+            _numPrices,
+            price1
+        );
+
+        // Create transfer message
+        BridgeUtils.Message memory message = BridgeUtils.Message({
+            messageType: BridgeUtils.ADD_EVM_TOKENS,
+            version: 1,
+            nonce: 0,
+            chainID: 1,
+            payload: payload
+        });
+
+        bytes memory encodedMessage = BridgeUtils.encodeMessage(message);
+
+        bytes32 messageHash = keccak256(encodedMessage);
+
+        bytes[] memory signatures = new bytes[](4);
+
+        signatures[0] = getSignature(messageHash, committeeMemberPkA);
+        signatures[1] = getSignature(messageHash, committeeMemberPkB);
+        signatures[2] = getSignature(messageHash, committeeMemberPkC);
+        signatures[3] = getSignature(messageHash, committeeMemberPkD);
+
+        // add token shoudl fail because the sui decimal is greater than the eth decimal
+        vm.expectRevert(bytes("BridgeConfig: Invalid Sui decimal"));
+        config.addTokensWithSignatures(signatures, message);
+    }
+
+    function testAddTokensPriceFailure() public {
+        MockUSDC _newToken = new MockUSDC();
+
+        // Create update tokens payload
+        bool _isNative = true;
+        uint8 _numTokenIDs = 1;
+        uint8 tokenID1 = 10;
+        uint8 _numAddresses = 1;
+        address address1 = address(_newToken);
+        uint8 _numSuiDecimals = 1;
+        uint8 suiDecimal1 = 10;
+        uint8 _numPrices = 1;
+        uint64 price1 = 0;
+
+        bytes memory payload = abi.encodePacked(
+            _isNative,
+            _numTokenIDs,
+            tokenID1,
+            _numAddresses,
+            address1,
+            _numSuiDecimals,
+            suiDecimal1,
+            _numPrices,
+            price1
+        );
+
+        // Create transfer message
+        BridgeUtils.Message memory message = BridgeUtils.Message({
+            messageType: BridgeUtils.ADD_EVM_TOKENS,
+            version: 1,
+            nonce: 0,
+            chainID: 1,
+            payload: payload
+        });
+
+        bytes memory encodedMessage = BridgeUtils.encodeMessage(message);
+
+        bytes32 messageHash = keccak256(encodedMessage);
+
+        bytes[] memory signatures = new bytes[](4);
+
+        signatures[0] = getSignature(messageHash, committeeMemberPkA);
+        signatures[1] = getSignature(messageHash, committeeMemberPkB);
+        signatures[2] = getSignature(messageHash, committeeMemberPkC);
+        signatures[3] = getSignature(messageHash, committeeMemberPkD);
+
+        vm.expectRevert(bytes("BridgeConfig: Invalid token price"));
+        config.addTokensWithSignatures(signatures, message);
+    }
+
     function testUpdateTokenPriceWithSignatures() public {
         // Create update tokens payload
         uint8 tokenID = BridgeUtils.ETH;
         uint64 price = 100_000_0000;
 
         bytes memory payload = abi.encodePacked(tokenID, price);
 
-        console.logBytes(payload);
-
         // Create transfer message
         BridgeUtils.Message memory message = BridgeUtils.Message({
             messageType: BridgeUtils.UPDATE_TOKEN_PRICE,

diff --git a/bridge/evm/test/mocks/MockSuiBridgeV2.sol b/bridge/evm/test/mocks/MockSuiBridgeV2.sol
@@ -21,5 +21,5 @@ contract MockSuiBridgeV2 is SuiBridge {
     }
 
     // used to ignore for forge coverage
-    function test() external view {}
+    function testSkip() external view {}
 }
diff --git a/bridge/evm/test/mocks/MockTokens.sol b/bridge/evm/test/mocks/MockTokens.sol
@@ -18,7 +18,7 @@ contract MockWBTC is ERC20 {
         return 8;
     }
 
-    function testMock() public {}
+    function testSkip() public {}
 }
 
 contract MockUSDC is ERC20 {
@@ -36,7 +36,7 @@ contract MockUSDC is ERC20 {
         return 6;
     }
 
-    function testMock() public {}
+    function testSkip() public {}
 }
 
 contract MockUSDT is ERC20 {
@@ -54,7 +54,7 @@ contract MockUSDT is ERC20 {
         return 6;
     }
 
-    function testMock() public {}
+    function testSkip() public {}
 }
 
 contract WETH {
@@ -112,5 +112,5 @@ contract WETH {
         return true;
     }
 
-    function testMock() public {}
+    function testSkip() public {}
 }