Audit fixes

contracts/.env.example

@@ -1,9 +1,21 @@
 # PROJECT ID from https://infura.io/dashboard
-INFURA_KEY=01117e8ede8e4f36801a6a838b24f36c
+INFURA_KEY=
 
-# Private keys
+# Private keys for deployers
+PRIVATEKEY_OWNER_AMB=
+PRIVATEKEY_OWNER_ETH=
+PRIVATEKEY_OWNER_BSC=
 
-# Deployer
-PRIVATEKEY_OWNER_AMB=34d8e83fca265e9ab5bcc1094fa64e98692375bf8980d066a9edcf4953f0f2f5
-PRIVATEKEY_OWNER_ETH=34d8e83fca265e9ab5bcc1094fa64e98692375bf8980d066a9edcf4953f0f2f5
-PRIVATEKEY_OWNER_BSC=34d8e83fca265e9ab5bcc1094fa64e98692375bf8980d066a9edcf4953f0f2f5
+PRIVATE_KEY_DEV=
+
+
+# verification apis
+ETHERSCAN_API_KEY=
+BSCSCAN_API_KEY=
+
+
+# for integration tests
+PRIVATEKEY_INTEGR1=
+PRIVATEKEY_INTEGR2=
+PRIVATEKEY_INTEGR3=
+PRIVATEKEY_INTEGR4=

contracts/contracts/checks/CheckUntrustless2.sol

@@ -6,6 +6,5 @@ import "../common/CommonStructs.sol";
 
 contract CheckUntrustless2 {
     // this contract do nothing, but i think it's required to inheritance from it
-    // todo check this
     uint256[15] private ___gap;
 }

contracts/contracts/common/CommonBridge.sol

@@ -7,10 +7,13 @@ import "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol";
 import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 import "./CommonStructs.sol";
 import "../tokens/IWrapper.sol";
-import "../checks/SignatureCheck.sol";
+import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
+import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
 
 
 contract CommonBridge is Initializable, AccessControlUpgradeable, PausableUpgradeable {
+    using SafeERC20 for IERC20;
+
     // DEFAULT_ADMIN_ROLE can grants and revokes all roles below; Set to multisig (proxy contract address)
     bytes32 public constant ADMIN_ROLE = keccak256("ADMIN_ROLE");  // can change tokens; unpause contract; change params like lockTime, minSafetyBlocks, ...
     bytes32 public constant RELAY_ROLE = keccak256("RELAY_ROLE");  // can submit transfers
@@ -95,16 +98,18 @@ contract CommonBridge is Initializable, AccessControlUpgradeable, PausableUpgrad
     /// @param feeSignature Signature signed by relay that confirms that the fee values are valid
     function wrapWithdraw(address toAddress,
         bytes calldata feeSignature, uint transferFee, uint bridgeFee
-    ) public payable {
+    ) public payable whenNotPaused {
         address tokenSideAddress = tokenAddresses[wrapperAddress];
         require(tokenSideAddress != address(0), "Unknown token address");
 
         require(msg.value > transferFee + bridgeFee, "Sent value <= fee");
 
         uint amount = msg.value - transferFee - bridgeFee;
         feeCheck(wrapperAddress, feeSignature, transferFee, bridgeFee, amount);
-        transferFeeRecipient.transfer(transferFee);
-        bridgeFeeRecipient.transfer(bridgeFee);
+        (bool sent, ) = payable(transferFeeRecipient).call{value: transferFee}("");
+        require(sent, "Transfer failed (transferFee)");
+        (sent, ) = payable(bridgeFeeRecipient).call{value: bridgeFee}("");
+        require(sent, "Transfer failed (bridgeFee)");
 
         IWrapper(wrapperAddress).deposit{value : amount}();
 
@@ -129,7 +134,7 @@ contract CommonBridge is Initializable, AccessControlUpgradeable, PausableUpgrad
     function withdraw(
         address tokenThisAddress, address toAddress, uint amount, bool unwrapSide,
         bytes calldata feeSignature, uint transferFee, uint bridgeFee
-    ) payable public {
+    ) payable public whenNotPaused {
         address tokenSideAddress;
         if (unwrapSide) {
             require(tokenAddresses[address(0)] == tokenThisAddress, "Token not point to native token");
@@ -144,10 +149,12 @@ contract CommonBridge is Initializable, AccessControlUpgradeable, PausableUpgrad
         require(amount > 0, "Cannot withdraw 0");
 
         feeCheck(tokenThisAddress, feeSignature, transferFee, bridgeFee, amount);
-        transferFeeRecipient.transfer(transferFee);
-        bridgeFeeRecipient.transfer(bridgeFee);
+        (bool sent, ) = payable(transferFeeRecipient).call{value: transferFee}("");
+        require(sent, "Transfer failed (transferFee)");
+        (sent, ) = payable(bridgeFeeRecipient).call{value: bridgeFee}("");
+        require(sent, "Transfer failed (bridgeFee)");
 
-        require(IERC20(tokenThisAddress).transferFrom(msg.sender, address(this), amount), "Fail transfer coins");
+        IERC20(tokenThisAddress).safeTransferFrom(msg.sender, address(this), amount);
 
         queue.push(CommonStructs.Transfer(tokenSideAddress, toAddress, amount));
         emit Withdraw(msg.sender, outputEventId, tokenThisAddress, tokenSideAddress, amount, transferFee, bridgeFee);
@@ -156,7 +163,7 @@ contract CommonBridge is Initializable, AccessControlUpgradeable, PausableUpgrad
     }
 
     // can be called to force emit `Transfer` event, without waiting for withdraw in next timeframe
-    function triggerTransfers() public {
+    function triggerTransfers() public whenNotPaused {
         require(queue.length != 0, "Queue is empty");
 
         emit Transfer(outputEventId++, queue);
@@ -172,12 +179,13 @@ contract CommonBridge is Initializable, AccessControlUpgradeable, PausableUpgrad
         require(transfersLocked.endTimestamp > 0, "no locked transfers with this id");
         require(transfersLocked.endTimestamp < block.timestamp, "lockTime has not yet passed");
 
-        proceedTransfers(transfersLocked.transfers);
-
         delete lockedTransfers[eventId];
         emit TransferFinish(eventId);
 
         oldestLockedEventId = eventId + 1;
+
+        // delete lockedTransfers[eventId] first to prevent reentrancy
+        proceedTransfers(transfersLocked.transfers);
     }
 
     // optimized version of unlockTransfers that unlock all transfer that can be unlocked in one call
@@ -187,10 +195,11 @@ contract CommonBridge is Initializable, AccessControlUpgradeable, PausableUpgrad
             CommonStructs.LockedTransfers memory transfersLocked = lockedTransfers[eventId];
             if (transfersLocked.endTimestamp == 0 || transfersLocked.endTimestamp > block.timestamp) break;
 
-            proceedTransfers(transfersLocked.transfers);
-
             delete lockedTransfers[eventId];
             emit TransferFinish(eventId);
+
+            // delete lockedTransfers[eventId] first to prevent reentrancy
+            proceedTransfers(transfersLocked.transfers);
         }
         oldestLockedEventId = eventId;
     }
@@ -315,6 +324,7 @@ contract CommonBridge is Initializable, AccessControlUpgradeable, PausableUpgrad
 
     // submitted transfers saves in `lockedTransfers` for `lockTime` period
     function lockTransfers(CommonStructs.Transfer[] calldata events, uint eventId) internal {
+        require(lockedTransfers[eventId].endTimestamp == 0, "lockedTransfers[eventId] not empty");
         lockedTransfers[eventId].endTimestamp = block.timestamp + lockTime;
         for (uint i = 0; i < events.length; i++)
             lockedTransfers[eventId].transfers.push(events[i]);
@@ -328,11 +338,10 @@ contract CommonBridge is Initializable, AccessControlUpgradeable, PausableUpgrad
 
             if (transfers[i].tokenAddress == address(0)) {// native token
                 IWrapper(wrapperAddress).withdraw(transfers[i].amount);
-                payable(transfers[i].toAddress).transfer(transfers[i].amount);
+                (bool sent, ) = payable(transfers[i].toAddress).call{value: transfers[i].amount}("");
+                require(sent, "Transfer failed");
             } else {// ERC20 token
-                require(
-                    IERC20(transfers[i].tokenAddress).transfer(transfers[i].toAddress, transfers[i].amount),
-                    "Fail transfer coins");
+                IERC20(transfers[i].tokenAddress).safeTransfer(transfers[i].toAddress, transfers[i].amount);
             }
 
         }
@@ -359,12 +368,11 @@ contract CommonBridge is Initializable, AccessControlUpgradeable, PausableUpgrad
         uint timestampEpoch = block.timestamp / SIGNATURE_FEE_TIMESTAMP;
 
         for (uint i = 0; i < signatureFeeCheckNumber; i++) {
-            messageHash = keccak256(abi.encodePacked(
-                    "\x19Ethereum Signed Message:\n32",
+            messageHash = ECDSA.toEthSignedMessageHash(
                     keccak256(abi.encodePacked(token, timestampEpoch, transferFee, bridgeFee, amount))
-                ));
+                );
 
-            signer = ecdsaRecover(messageHash, signature);
+            (signer, ) = ECDSA.tryRecover(messageHash, signature);
             if (hasRole(FEE_PROVIDER_ROLE, signer))
                 return;
             timestampEpoch--;

contracts/contracts/contracts_for_tests/CommonBridgeTest.sol

@@ -2,6 +2,7 @@
 pragma solidity 0.8.6;
 
 import "../common/CommonBridge.sol";
+import "hardhat/console.sol";
 
 contract CommonBridgeTest is CommonBridge {
 
@@ -24,7 +25,7 @@ contract CommonBridgeTest is CommonBridge {
 
 
     function checkSignatureTest(bytes32 hash, bytes memory signature) public view returns(address) {
-        return ecdsaRecover(hash, signature);
+        return ECDSA.recover(hash, signature);
     }
 
 

contracts/contracts/networks/BSC_AmbBridge.sol

@@ -13,30 +13,6 @@ contract BSC_AmbBridge is CommonBridge, CheckUntrustless2 {
         __CommonBridge_init(args);
     }
 
-    function upgrade(
-        address[] calldata _watchdogs,
-        address _fee_provider,
-        address mpcRelay,
-        address oldDefaultAdmin,
-        address oldRelay
-    ) public {
-        require(msg.sender == address(this), "This method require multisig");
-
-        // add DEFAULT_ADMIN_ROLE to multisig
-        _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
-
-        // revoke RELAY_ROLE from old relay
-        revokeRole(RELAY_ROLE, oldRelay);
-
-        // new roles for untrustless mpc
-        _setupRoles(WATCHDOG_ROLE, _watchdogs);
-        _setupRole(FEE_PROVIDER_ROLE, _fee_provider);
-        _setupRole(RELAY_ROLE, mpcRelay);
-
-        // revoke DEFAULT_ADMIN_ROLE from deployer
-        revokeRole(DEFAULT_ADMIN_ROLE, oldDefaultAdmin);
-    }
-
 
     function submitTransferUntrustless(uint eventId, CommonStructs.Transfer[] calldata transfers) public onlyRole(RELAY_ROLE) whenNotPaused {
         checkEventId(eventId);

contracts/contracts/networks/BSC_BscBridge.sol

@@ -13,29 +13,6 @@ contract BSC_BscBridge is CommonBridge, CheckUntrustless2 {
         __CommonBridge_init(args);
     }
 
-    function upgrade(
-        address[] calldata _watchdogs,
-        address _fee_provider,
-        address mpcRelay,
-        address oldDefaultAdmin,
-        address oldRelay
-    ) public {
-        require(msg.sender == address(this), "This method require multisig");
-
-        // add DEFAULT_ADMIN_ROLE to multisig
-        _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
-
-        // revoke RELAY_ROLE from old relay
-        revokeRole(RELAY_ROLE, oldRelay);
-
-        // new roles for untrustless mpc
-        _setupRoles(WATCHDOG_ROLE, _watchdogs);
-        _setupRole(FEE_PROVIDER_ROLE, _fee_provider);
-        _setupRole(RELAY_ROLE, mpcRelay);
-
-        // revoke DEFAULT_ADMIN_ROLE from deployer
-        revokeRole(DEFAULT_ADMIN_ROLE, oldDefaultAdmin);
-    }
 
     function submitTransferUntrustless(uint eventId, CommonStructs.Transfer[] calldata transfers) public onlyRole(RELAY_ROLE) whenNotPaused {
         checkEventId(eventId);

contracts/contracts/networks/ETH_AmbBridge.sol

@@ -13,29 +13,6 @@ contract ETH_AmbBridge is CommonBridge, CheckUntrustless2 {
         __CommonBridge_init(args);
     }
 
-    function upgrade(
-        address[] calldata _watchdogs,
-        address _fee_provider,
-        address mpcRelay,
-        address oldDefaultAdmin,
-        address oldRelay
-    ) public {
-        require(msg.sender == address(this), "This method require multisig");
-
-        // add DEFAULT_ADMIN_ROLE to multisig
-        _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
-
-        // revoke RELAY_ROLE from old relay
-        revokeRole(RELAY_ROLE, oldRelay);
-
-        // new roles for untrustless mpc
-        _setupRoles(WATCHDOG_ROLE, _watchdogs);
-        _setupRole(FEE_PROVIDER_ROLE, _fee_provider);
-        _setupRole(RELAY_ROLE, mpcRelay);
-
-        // revoke DEFAULT_ADMIN_ROLE from deployer
-        revokeRole(DEFAULT_ADMIN_ROLE, oldDefaultAdmin);
-    }
 
 
     function submitTransferUntrustless(uint eventId, CommonStructs.Transfer[] calldata transfers) public onlyRole(RELAY_ROLE) whenNotPaused {

contracts/contracts/networks/ETH_EthBridge.sol

@@ -13,22 +13,6 @@ contract ETH_EthBridge is CommonBridge, CheckUntrustless2 {
         __CommonBridge_init(args);
     }
 
-    function upgrade(
-        address[] calldata _watchdogs,
-        address _fee_provider,
-        address oldDefaultAdmin
-    ) public {
-        require(msg.sender == address(this), "This method require multisig");
-
-        // new roles for untrustless mpc
-        _setupRoles(WATCHDOG_ROLE, _watchdogs);
-        _setupRole(FEE_PROVIDER_ROLE, _fee_provider);
-
-        // add DEFAULT_ADMIN_ROLE to multisig
-        _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
-        // revoke DEFAULT_ADMIN_ROLE from deployer
-        revokeRole(DEFAULT_ADMIN_ROLE, oldDefaultAdmin);
-    }
 
     function submitTransferUntrustless(uint eventId, CommonStructs.Transfer[] calldata transfers) public onlyRole(RELAY_ROLE) whenNotPaused {
         checkEventId(eventId);

contracts/contracts/tokens/BridgeERC20_Amb.sol

@@ -34,7 +34,6 @@ contract BridgeERC20_Amb is ERC20, Ownable {
         _setSideTokenDecimals(bridgeAddresses_, sideTokenDecimals_);
     }
 
-    // todo check if we need this func
     function _setSideTokenDecimals(address[] memory bridgeAddresses_, uint8[] memory sideTokenDecimals_) private {
         require(bridgeAddresses_.length == sideTokenDecimals_.length, "wrong array lengths");
         for (uint i = 0; i < bridgeAddresses_.length; i++)