impl reetrency guards (#52)

src/child/ChildERC20Bridge.sol

@@ -11,6 +11,7 @@ import {
 } from "../interfaces/child/IChildERC20Bridge.sol";
 import {IChildBridgeAdaptor} from "../interfaces/child/IChildBridgeAdaptor.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";
 
@@ -39,7 +40,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;
 
@@ -101,6 +108,7 @@ contract ChildERC20Bridge is BridgeRoles, IChildERC20BridgeErrors, IChildERC20Br
 
         __AccessControl_init();
         __Pausable_init();
+        __ReentrancyGuard_init();
 
         _grantRole(DEFAULT_ADMIN_ROLE, newRoles.defaultAdmin);
         _grantRole(PAUSER_ROLE, newRoles.pauser);
@@ -168,7 +176,7 @@ contract ChildERC20Bridge is BridgeRoles, IChildERC20BridgeErrors, IChildERC20Br
      *      This method assumes that the adaptor will have performed all
      *      validations relating to the source of the message, prior to calling this method.
      */
-    function onMessageReceive(bytes calldata data) external override whenNotPaused onlyBridgeAdaptor {
+    function onMessageReceive(bytes calldata data) external whenNotPaused onlyBridgeAdaptor {
         if (data.length <= 32) {
             // Data must always be greater than 32.
             // 32 bytes for the signature, and at least some information for the payload
@@ -257,7 +265,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();
         }

src/root/RootERC20Bridge.sol

@@ -5,7 +5,7 @@ pragma solidity 0.8.19;
 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 "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol";
+import {ReentrancyGuardUpgradeable} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
 import {AccessControlUpgradeable} from "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol";
 import {
     IRootERC20Bridge,
@@ -44,7 +44,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.
@@ -148,6 +154,7 @@ contract RootERC20Bridge is BridgeRoles, IRootERC20Bridge, IRootERC20BridgeEvent
 
         __AccessControl_init();
         __Pausable_init();
+        __ReentrancyGuard_init();
 
         _grantRole(DEFAULT_ADMIN_ROLE, newRoles.defaultAdmin);
         _grantRole(PAUSER_ROLE, newRoles.pauser);
@@ -376,6 +383,7 @@ contract RootERC20Bridge is BridgeRoles, IRootERC20Bridge, IRootERC20BridgeEvent
 
     function _deposit(IERC20Metadata rootToken, address receiver, uint256 amount)
         private
+        nonReentrant
         whenNotPaused
         wontIMXOverflow(address(rootToken), amount)
     {

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,
@@ -107,7 +105,6 @@ contract RootERC20BridgeFlowRate is
         );
 
         __FlowRateWithdrawalQueue_init();
-        __ReentrancyGuard_init();
         _grantRole(RATE_CONTROL_ROLE, rateAdmin);
     }
 

test/integration/child/ChildAxelarBridge.t.sol

@@ -307,7 +307,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)));
@@ -334,7 +334,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)));

test/unit/child/ChildERC20Bridge.t.sol

@@ -14,6 +14,22 @@ import {
 import {ChildERC20} from "../../../src/child/ChildERC20.sol";
 import {Utils, IPausable} from "../../utils.t.sol";
 
+contract ReentrancyAttackWithdraw is ChildERC20 {
+    IChildERC20Bridge bridge;
+
+    constructor(address _bridge) {
+        bridge = IChildERC20Bridge(_bridge);
+    }
+
+    function burn(address from, uint256 value) public override returns (bool) {
+        if (msg.sender == address(bridge)) {
+            bridge.withdraw(ChildERC20(address(this)), value);
+        }
+        _burn(from, value);
+        return true;
+    }
+}
+
 contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20BridgeErrors, Utils {
     address constant ROOT_BRIDGE = address(3);
     address constant ROOT_IMX_TOKEN = address(0xccc);
@@ -54,7 +70,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 = 256;
+        uint256 wIMXStorageSlot = 306;
         vm.store(address(childBridge), bytes32(wIMXStorageSlot), bytes32(uint256(uint160(caller))));
 
         vm.startPrank(caller);
@@ -570,7 +586,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)));
@@ -582,4 +598,35 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B
         vm.expectRevert(EmptyTokenContract.selector);
         childBridge.onMessageReceive(depositData);
     }
+
+    /**
+     * WITHDRAW
+     */
+
+    function test_RevertIf_WithdrawReentered() public {
+        // Create attack token
+        vm.startPrank(address(childBridge));
+        ReentrancyAttackWithdraw attackToken = new ReentrancyAttackWithdraw(address(childBridge));
+        address rootAddr = makeAddr("rootAddr");
+        attackToken.initialize(rootAddr, "Test", "TST", 18);
+
+        // Map
+        {
+            // Found by running `forge inspect src/child/ChildERC20Bridge.sol:ChildERC20Bridge storageLayout | grep -B3 -A5 -i "rootTokenToChildToken"`
+            uint256 rootTokenToChildTokenMappingSlot = 301;
+            bytes32 slot = getMappingStorageSlotFor(rootAddr, rootTokenToChildTokenMappingSlot);
+            bytes32 data = bytes32(uint256(uint160(address(attackToken))));
+            vm.store(address(childBridge), slot, data);
+        }
+
+        // Create attacker
+        address attacker = makeAddr("attacker");
+        attackToken.mint(attacker, 1000);
+        vm.deal(attacker, 10 ether);
+        changePrank(attacker);
+
+        // Execute withdraw
+        vm.expectRevert("ReentrancyGuard: reentrant call");
+        childBridge.withdraw{value: 1 ether}(ChildERC20(address(attackToken)), 100);
+    }
 }

test/unit/root/RootERC20Bridge.t.sol

@@ -18,6 +18,22 @@ import {MockAxelarGasService} from "../../mocks/root/MockAxelarGasService.sol";
 import {MockAdaptor} from "../../mocks/root/MockAdaptor.sol";
 import {Utils, IPausable} from "../../utils.t.sol";
 
+contract ReentrancyAttackDeposit is ERC20PresetMinterPauser {
+    IRootERC20Bridge bridge;
+
+    constructor(address _bridge) ERC20PresetMinterPauser("Test", "TST") {
+        bridge = IRootERC20Bridge(_bridge);
+    }
+
+    function transferFrom(address from, address to, uint256 amount) public override returns (bool) {
+        if (msg.sender == address(bridge)) {
+            bridge.deposit(IERC20Metadata(address(this)), amount);
+        }
+
+        return super.transferFrom(from, to, amount);
+    }
+}
+
 contract RootERC20BridgeUnitTest is Test, IRootERC20BridgeEvents, IRootERC20BridgeErrors, Utils {
     address constant CHILD_BRIDGE = address(3);
     address constant IMX_TOKEN = address(0xccc);
@@ -85,7 +101,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 = 257;
+        uint256 wETHStorageSlot = 307;
         vm.store(address(rootBridge), bytes32(wETHStorageSlot), bytes32(uint256(uint160(caller))));
 
         vm.startPrank(caller);
@@ -677,6 +693,31 @@ contract RootERC20BridgeUnitTest is Test, IRootERC20BridgeEvents, IRootERC20Brid
      * DEPOSIT TOKEN
      */
 
+    function test_RevertsIf_DepositReentered() public {
+        // Create attack token
+        ReentrancyAttackDeposit attackToken = new ReentrancyAttackDeposit(address(rootBridge));
+
+        // Map
+        {
+            // Found by running `forge inspect src/root/RootERC20Bridge.sol:RootERC20Bridge storageLayout | grep -B3 -A5 -i "rootTokenToChildToken"`
+            uint256 rootTokenToChildTokenMappingSlot = 301;
+            bytes32 slot = getMappingStorageSlotFor(address(attackToken), rootTokenToChildTokenMappingSlot);
+            bytes32 data = bytes32(uint256(uint160(address(attackToken))));
+            vm.store(address(rootBridge), slot, data);
+        }
+
+        // Approve and mint
+        address attacker = makeAddr("attacker");
+        ERC20PresetMinterPauser(address(attackToken)).mint(attacker, 10000);
+        vm.startPrank(attacker);
+        vm.deal(attacker, 10 ether);
+        ERC20PresetMinterPauser(address(attackToken)).approve(address(rootBridge), type(uint256).max);
+
+        // Deposit
+        vm.expectRevert("ReentrancyGuard: reentrant call");
+        rootBridge.deposit{value: depositFee}(IERC20Metadata(address(attackToken)), 100);
+    }
+
     function test_RevertsIf_DepositTokenWhenPaused() public {
         uint256 amount = 100;
         pause(IPausable(address(rootBridge)));