diff --git a/src/child/ChildERC20Bridge.sol b/src/child/ChildERC20Bridge.sol index ced13e94..c10a816d 100644 --- a/src/child/ChildERC20Bridge.sol +++ b/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(); } diff --git a/src/root/RootERC20Bridge.sol b/src/root/RootERC20Bridge.sol index e93f8eb7..4a7678e5 100644 --- a/src/root/RootERC20Bridge.sol +++ b/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) { diff --git a/src/root/flowrate/RootERC20BridgeFlowRate.sol b/src/root/flowrate/RootERC20BridgeFlowRate.sol index ce1f6147..dd7bf0e0 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, @@ -107,7 +105,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 28f683f9..b9b15e63 100644 --- a/test/integration/child/ChildAxelarBridge.t.sol +++ b/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))); diff --git a/test/unit/child/ChildERC20Bridge.t.sol b/test/unit/child/ChildERC20Bridge.t.sol index 341237bd..63f43467 100644 --- a/test/unit/child/ChildERC20Bridge.t.sol +++ b/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); + } } diff --git a/test/unit/root/RootERC20Bridge.t.sol b/test/unit/root/RootERC20Bridge.t.sol index 95e2b951..586cef74 100644 --- a/test/unit/root/RootERC20Bridge.t.sol +++ b/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)));