From a6236e8372e10eb1388306c08e0728c749fbca55 Mon Sep 17 00:00:00 2001 From: James Snewin Date: Thu, 23 Nov 2023 09:51:52 +1000 Subject: [PATCH] impl reetrency guards --- src/child/ChildERC20Bridge.sol | 13 +++++++++++-- src/root/RootERC20Bridge.sol | 14 +++++++++++--- src/root/flowrate/RootERC20BridgeFlowRate.sol | 3 --- test/integration/child/ChildAxelarBridge.t.sol | 4 ++-- test/unit/child/ChildERC20Bridge.t.sol | 4 ++-- test/unit/root/RootERC20Bridge.t.sol | 2 +- 6 files changed, 27 insertions(+), 13 deletions(-) diff --git a/src/child/ChildERC20Bridge.sol b/src/child/ChildERC20Bridge.sol index 1c3e60db..125812bf 100644 --- 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 index c38e2bbd..b901bca2 100644 --- 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 index 584609ed..f2486da1 100644 --- 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 index 408a0d8e..ef92151d 100644 --- 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 index 90c8b6d0..861271e5 100644 --- 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 index c1406a12..63fa54cd 100644 --- 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);