impl reetrency guards

immutable · Nov 22, 2023 · a6236e8 · a6236e8
1 parent 5e5e2c2
commit a6236e8
Show file tree

Hide file tree

Showing 6 changed files with 27 additions and 13 deletions.
diff --git a/src/child/ChildERC20Bridge.sol b/src/child/ChildERC20Bridge.sol
@@ -12,6 +12,7 @@ import {
 } from "../interfaces/child/IChildERC20Bridge.sol";
 import {IChildERC20BridgeAdaptor} from "../interfaces/child/IChildERC20BridgeAdaptor.sol";
 import {IChildERC20} from "../interfaces/child/IChildERC20.sol";
+import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
 import {IWIMX} from "../interfaces/child/IWIMX.sol";
 import {BridgeRoles} from "../common/BridgeRoles.sol";
 
@@ -40,7 +41,13 @@ import {BridgeRoles} from "../common/BridgeRoles.sol";
  *      - This is an upgradeable contract that should be operated behind OpenZeppelin's TransparentUpgradeableProxy.
  *      - The initialize function is susceptible to front running, so precautions should be taken to account for this scenario.
  */
-contract ChildERC20Bridge is BridgeRoles, IChildERC20BridgeErrors, IChildERC20Bridge, IChildERC20BridgeEvents {
+contract ChildERC20Bridge is
+    BridgeRoles,
+    ReentrancyGuardUpgradeable,
+    IChildERC20BridgeErrors,
+    IChildERC20Bridge,
+    IChildERC20BridgeEvents
+{
     /// @dev leave this as the first param for the integration tests.
     mapping(address => address) public rootTokenToChildToken;
 
@@ -107,6 +114,7 @@ contract ChildERC20Bridge is BridgeRoles, IChildERC20BridgeErrors, IChildERC20Br
 
         __AccessControl_init();
         __Pausable_init();
+        __ReentrancyGuard_init();
 
         _grantRole(DEFAULT_ADMIN_ROLE, newRoles.defaultAdmin);
         _grantRole(PAUSER_ROLE, newRoles.pauser);
@@ -188,6 +196,7 @@ contract ChildERC20Bridge is BridgeRoles, IChildERC20BridgeErrors, IChildERC20Br
     function onMessageReceive(string calldata messageSourceChain, string calldata sourceAddress, bytes calldata data)
         external
         override
+        nonReentrant
         whenNotPaused
     {
         if (msg.sender != address(bridgeAdaptor)) {
@@ -286,7 +295,7 @@ contract ChildERC20Bridge is BridgeRoles, IChildERC20BridgeErrors, IChildERC20Br
      * - `childToken` must be mapped.
      * - `childToken` must have a the bridge set.
      */
-    function _withdraw(address childTokenAddr, address receiver, uint256 amount) private whenNotPaused {
+    function _withdraw(address childTokenAddr, address receiver, uint256 amount) private nonReentrant whenNotPaused {
         if (childTokenAddr == address(0) || receiver == address(0)) {
             revert ZeroAddress();
         }

diff --git a/src/root/RootERC20Bridge.sol b/src/root/RootERC20Bridge.sol
@@ -6,8 +6,6 @@ import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol
 import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol";
 import {Address} from "@openzeppelin/contracts/utils/Address.sol";
 import {Strings} from "@openzeppelin/contracts/utils/Strings.sol";
-import "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol";
-import {AccessControlUpgradeable} from "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol";
 import {IAxelarGateway} from "@axelar-cgp-solidity/contracts/interfaces/IAxelarGateway.sol";
 import {
     IRootERC20Bridge,
@@ -16,6 +14,7 @@ import {
     IRootERC20BridgeErrors
 } from "../interfaces/root/IRootERC20Bridge.sol";
 import {IRootERC20BridgeAdaptor} from "../interfaces/root/IRootERC20BridgeAdaptor.sol";
+import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
 import {IWETH} from "../interfaces/root/IWETH.sol";
 import {BridgeRoles} from "../common/BridgeRoles.sol";
 
@@ -46,7 +45,13 @@ import {BridgeRoles} from "../common/BridgeRoles.sol";
  *      - This is an upgradeable contract that should be operated behind OpenZeppelin's TransparentUpgradeableProxy.
  *      - The initialize function is susceptible to front running, so precautions should be taken to account for this scenario.
  */
-contract RootERC20Bridge is BridgeRoles, IRootERC20Bridge, IRootERC20BridgeEvents, IRootERC20BridgeErrors {
+contract RootERC20Bridge is
+    BridgeRoles,
+    ReentrancyGuardUpgradeable,
+    IRootERC20Bridge,
+    IRootERC20BridgeEvents,
+    IRootERC20BridgeErrors
+{
     using SafeERC20 for IERC20Metadata;
 
     /// @dev leave this as the first param for the integration tests.
@@ -158,6 +163,7 @@ contract RootERC20Bridge is BridgeRoles, IRootERC20Bridge, IRootERC20BridgeEvent
 
         __AccessControl_init();
         __Pausable_init();
+        __ReentrancyGuard_init();
 
         _grantRole(DEFAULT_ADMIN_ROLE, newRoles.defaultAdmin);
         _grantRole(PAUSER_ROLE, newRoles.pauser);
@@ -261,6 +267,7 @@ contract RootERC20Bridge is BridgeRoles, IRootERC20Bridge, IRootERC20BridgeEvent
     function onMessageReceive(string calldata messageSourceChain, string calldata sourceAddress, bytes calldata data)
         external
         override
+        nonReentrant
         whenNotPaused
     {
         if (msg.sender != address(rootBridgeAdaptor)) {
@@ -411,6 +418,7 @@ contract RootERC20Bridge is BridgeRoles, IRootERC20Bridge, IRootERC20BridgeEvent
 
     function _deposit(IERC20Metadata rootToken, address receiver, uint256 amount)
         private
+        nonReentrant
         whenNotPaused
         wontIMXOverflow(address(rootToken), amount)
     {

diff --git a/src/root/flowrate/RootERC20BridgeFlowRate.sol b/src/root/flowrate/RootERC20BridgeFlowRate.sol
@@ -2,7 +2,6 @@
 // SPDX-License-Identifier: Apache 2.0
 pragma solidity 0.8.19;
 
-import "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
 import "./FlowRateDetection.sol";
 import "./FlowRateWithdrawalQueue.sol";
 import "../RootERC20Bridge.sol";
@@ -69,7 +68,6 @@ import {
  */
 contract RootERC20BridgeFlowRate is
     RootERC20Bridge,
-    ReentrancyGuardUpgradeable,
     FlowRateDetection,
     FlowRateWithdrawalQueue,
     IRootERC20BridgeFlowRateEvents,
@@ -111,7 +109,6 @@ contract RootERC20BridgeFlowRate is
         );
 
         __FlowRateWithdrawalQueue_init();
-        __ReentrancyGuard_init();
         _grantRole(RATE_CONTROL_ROLE, rateAdmin);
     }
 

diff --git a/test/integration/child/ChildAxelarBridge.t.sol b/test/integration/child/ChildAxelarBridge.t.sol
@@ -306,7 +306,7 @@ contract ChildERC20BridgeIntegrationTest is Test, IChildERC20BridgeEvents, IChil
 
         {
             // Found by running `forge inspect src/child/ChildERC20Bridge.sol:ChildERC20Bridge storageLayout | grep -B3 -A5 -i "rootTokenToChildToken"`
-            uint256 rootTokenToChildTokenMappingSlot = 201;
+            uint256 rootTokenToChildTokenMappingSlot = 301;
             address childAddress = address(444444);
             bytes32 slot = getMappingStorageSlotFor(rootTokenAddress, rootTokenToChildTokenMappingSlot);
             bytes32 data = bytes32(uint256(uint160(childAddress)));
@@ -333,7 +333,7 @@ contract ChildERC20BridgeIntegrationTest is Test, IChildERC20BridgeEvents, IChil
         address rootAddress = address(0x123);
         {
             // Found by running `forge inspect src/child/ChildERC20Bridge.sol:ChildERC20Bridge storageLayout | grep -B3 -A5 -i "rootTokenToChildToken"`
-            uint256 rootTokenToChildTokenMappingSlot = 251;
+            uint256 rootTokenToChildTokenMappingSlot = 301;
             address childAddress = address(444444);
             bytes32 slot = getMappingStorageSlotFor(rootAddress, rootTokenToChildTokenMappingSlot);
             bytes32 data = bytes32(uint256(uint160(childAddress)));

diff --git a/test/unit/child/ChildERC20Bridge.t.sol b/test/unit/child/ChildERC20Bridge.t.sol
@@ -64,7 +64,7 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B
         address caller = address(0x123a);
         payable(caller).transfer(2 ether);
         // forge inspect src/child/ChildERC20Bridge.sol:ChildERC20Bridge storageLayout | grep -B3 -A5 -i "wIMXToken"
-        uint256 wIMXStorageSlot = 258;
+        uint256 wIMXStorageSlot = 308;
         vm.store(address(childBridge), bytes32(wIMXStorageSlot), bytes32(uint256(uint160(caller))));
 
         vm.startPrank(caller);
@@ -665,7 +665,7 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B
         address rootAddress = address(0x123);
         {
             // Found by running `forge inspect src/child/ChildERC20Bridge.sol:ChildERC20Bridge storageLayout | grep -B3 -A5 -i "rootTokenToChildToken"`
-            uint256 rootTokenToChildTokenMappingSlot = 251;
+            uint256 rootTokenToChildTokenMappingSlot = 301;
             address childAddress = address(444444);
             bytes32 slot = getMappingStorageSlotFor(rootAddress, rootTokenToChildTokenMappingSlot);
             bytes32 data = bytes32(uint256(uint160(childAddress)));

diff --git a/test/unit/root/RootERC20Bridge.t.sol b/test/unit/root/RootERC20Bridge.t.sol
@@ -93,7 +93,7 @@ contract RootERC20BridgeUnitTest is Test, IRootERC20BridgeEvents, IRootERC20Brid
         address caller = address(0x123a);
         payable(caller).transfer(2 ether);
         // forge inspect src/root/RootERC20Bridge.sol:RootERC20Bridge storageLayout | grep -B3 -A5 -i "rootWETHToken"
-        uint256 wETHStorageSlot = 258;
+        uint256 wETHStorageSlot = 308;
         vm.store(address(rootBridge), bytes32(wETHStorageSlot), bytes32(uint256(uint160(caller))));
 
         vm.startPrank(caller);