From b9409a5a75dc87dbc4df9db23e47cf5ec14530f9 Mon Sep 17 00:00:00 2001 From: MaxMustermann2 <82761650+MaxMustermann2@users.noreply.github.com> Date: Tue, 11 Jun 2024 01:15:02 +0000 Subject: [PATCH] feat(bootstrap): add reentrancy guards --- src/core/Bootstrap.sol | 76 ++++++++++++++++++++++++------------ src/core/ExocoreGateway.sol | 2 + test/foundry/Bootstrap.t.sol | 18 +++++++++ 3 files changed, 71 insertions(+), 25 deletions(-) diff --git a/src/core/Bootstrap.sol b/src/core/Bootstrap.sol index 0b2db35b..2efe5807 100644 --- a/src/core/Bootstrap.sol +++ b/src/core/Bootstrap.sol @@ -7,6 +7,7 @@ import {ITransparentUpgradeableProxy} from import {OwnableUpgradeable} from "@openzeppelin-upgradeable/contracts/access/OwnableUpgradeable.sol"; import {Initializable} from "@openzeppelin-upgradeable/contracts/proxy/utils/Initializable.sol"; import {PausableUpgradeable} from "@openzeppelin-upgradeable/contracts/utils/PausableUpgradeable.sol"; +import {ReentrancyGuardUpgradeable} from "@openzeppelin-upgradeable/contracts/utils/ReentrancyGuardUpgradeable.sol"; import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; import {OAppCoreUpgradeable} from "../lzApp/OAppCoreUpgradeable.sol"; @@ -28,6 +29,7 @@ contract Bootstrap is Initializable, PausableUpgradeable, OwnableUpgradeable, + ReentrancyGuardUpgradeable, ILSTRestakingController, IOperatorRegistry, BootstrapLzReceiver @@ -156,7 +158,7 @@ contract Bootstrap is } // implementation of ITokenWhitelister - function addWhitelistToken(address _token) public override beforeLocked onlyOwner whenNotPaused { + function addWhitelistToken(address _token) public override beforeLocked onlyOwner whenNotPaused nonReentrant { super.addWhitelistToken(_token); } @@ -314,26 +316,32 @@ contract Bootstrap is whenNotPaused isTokenWhitelisted(token) isValidAmount(amount) + nonReentrant // interacts with Vault { + _deposit(msg.sender, token, amount); + } + + // _deposit is the internal function that does the work + function _deposit(address depositor, address token, uint256 amount) internal { IVault vault = _getVault(token); - vault.deposit(msg.sender, amount); + vault.deposit(depositor, amount); - if (!isDepositor[msg.sender]) { - isDepositor[msg.sender] = true; - depositors.push(msg.sender); + if (!isDepositor[depositor]) { + isDepositor[depositor] = true; + depositors.push(depositor); } // staker_asset.go duplicate here. the duplication is required (and not simply inferred // from vault) because the vault is not altered by the gateway in response to // delegations or undelegations. hence, this is not something we can do either. - totalDepositAmounts[msg.sender][token] += amount; - withdrawableAmounts[msg.sender][token] += amount; + totalDepositAmounts[depositor][token] += amount; + withdrawableAmounts[depositor][token] += amount; depositsByToken[token] += amount; // afterReceiveDepositResponse stores the TotalDepositAmount in the principle. - vault.updatePrincipleBalance(msg.sender, totalDepositAmounts[msg.sender][token]); + vault.updatePrincipleBalance(depositor, totalDepositAmounts[depositor][token]); - emit DepositResult(true, token, msg.sender, amount); + emit DepositResult(true, token, depositor, amount); } // implementation of ILSTRestakingController @@ -346,24 +354,30 @@ contract Bootstrap is whenNotPaused isTokenWhitelisted(token) isValidAmount(amount) + nonReentrant // interacts with Vault { + _withdraw(msg.sender, token, amount); + } + + // _withdraw is the internal function that does the actual work. + function _withdraw(address user, address token, uint256 amount) internal { IVault vault = _getVault(token); - uint256 deposited = totalDepositAmounts[msg.sender][token]; + uint256 deposited = totalDepositAmounts[user][token]; require(deposited >= amount, "Bootstrap: insufficient deposited balance"); - uint256 withdrawable = withdrawableAmounts[msg.sender][token]; + uint256 withdrawable = withdrawableAmounts[user][token]; require(withdrawable >= amount, "Bootstrap: insufficient withdrawable balance"); // when the withdraw precompile is called, it does these things. - totalDepositAmounts[msg.sender][token] -= amount; - withdrawableAmounts[msg.sender][token] -= amount; + totalDepositAmounts[user][token] -= amount; + withdrawableAmounts[user][token] -= amount; depositsByToken[token] -= amount; // afterReceiveWithdrawPrincipleResponse - vault.updatePrincipleBalance(msg.sender, totalDepositAmounts[msg.sender][token]); - vault.updateWithdrawableBalance(msg.sender, amount, 0); + vault.updatePrincipleBalance(user, totalDepositAmounts[user][token]); + vault.updateWithdrawableBalance(user, amount, 0); - emit WithdrawPrincipleResult(true, token, msg.sender, amount); + emit WithdrawPrincipleResult(true, token, user, amount); } // implementation of ILSTRestakingController @@ -380,6 +394,7 @@ contract Bootstrap is whenNotPaused isTokenWhitelisted(token) isValidAmount(amount) + nonReentrant // because it interacts with vault { IVault vault = _getVault(token); vault.withdraw(msg.sender, recipient, amount); @@ -395,7 +410,12 @@ contract Bootstrap is isTokenWhitelisted(token) isValidAmount(amount) isValidBech32Address(operator) + // does not need a reentrancy guard { + _delegateTo(msg.sender, operator, token, amount); + } + + function _delegateTo(address user, string calldata operator, address token, uint256 amount) internal { require(msg.value == 0, "Bootstrap: no ether required for delegation"); // check that operator is registered require(bytes(operators[operator].name).length != 0, "Operator does not exist"); @@ -404,11 +424,11 @@ contract Bootstrap is // now check amounts. uint256 withdrawable = withdrawableAmounts[msg.sender][token]; require(withdrawable >= amount, "Bootstrap: insufficient withdrawable balance"); - delegations[msg.sender][operator][token] += amount; + delegations[user][operator][token] += amount; delegationsByOperator[operator][token] += amount; - withdrawableAmounts[msg.sender][token] -= amount; + withdrawableAmounts[user][token] -= amount; - emit DelegateResult(true, msg.sender, operator, token, amount); + emit DelegateResult(true, user, operator, token, amount); } // implementation of ILSTRestakingController @@ -421,21 +441,26 @@ contract Bootstrap is isTokenWhitelisted(token) isValidAmount(amount) isValidBech32Address(operator) + // does not need a reentrancy guard { + _undelegateFrom(msg.sender, operator, token, amount); + } + + function _undelegateFrom(address user, string calldata operator, address token, uint256 amount) internal { require(msg.value == 0, "Bootstrap: no ether required for undelegation"); // check that operator is registered require(bytes(operators[operator].name).length != 0, "Operator does not exist"); // operator can't be frozen and amount can't be negative // asset validity has been checked. // now check amounts. - uint256 delegated = delegations[msg.sender][operator][token]; + uint256 delegated = delegations[user][operator][token]; require(delegated >= amount, "Bootstrap: insufficient delegated balance"); // the undelegation is released immediately since it is not at stake yet. - delegations[msg.sender][operator][token] -= amount; + delegations[user][operator][token] -= amount; delegationsByOperator[operator][token] -= amount; - withdrawableAmounts[msg.sender][token] += amount; + withdrawableAmounts[user][token] += amount; - emit UndelegateResult(true, msg.sender, operator, token, amount); + emit UndelegateResult(true, user, operator, token, amount); } // implementation of ILSTRestakingController @@ -448,9 +473,10 @@ contract Bootstrap is isTokenWhitelisted(token) isValidAmount(amount) isValidBech32Address(operator) + nonReentrant // because it interacts with vault in deposit { - this.deposit(token, amount); - this.delegateTo(operator, token, amount); + _deposit(msg.sender, token, amount); + _delegateTo(msg.sender, operator, token, amount); } /** diff --git a/src/core/ExocoreGateway.sol b/src/core/ExocoreGateway.sol index 2c0069dd..fc98e500 100644 --- a/src/core/ExocoreGateway.sol +++ b/src/core/ExocoreGateway.sol @@ -237,6 +237,8 @@ contract ExocoreGateway is // while some of the code from requestDeposit and requestDelegateTo is duplicated here, // it is done intentionally to work around Solidity's limitations with regards to // function calls, error handling and indexing the return data of memory type. + // for example, you cannot index a bytes memory result from the requestDepositTo call, + // if you were to modify it to return bytes and then process them here. (bool success, uint256 updatedBalance) = DEPOSIT_CONTRACT.depositTo(srcChainId, token, depositor, amount); if (!success) { diff --git a/test/foundry/Bootstrap.t.sol b/test/foundry/Bootstrap.t.sol index ef562f17..a443222c 100644 --- a/test/foundry/Bootstrap.t.sol +++ b/test/foundry/Bootstrap.t.sol @@ -414,6 +414,24 @@ contract BootstrapTest is Test { vm.stopPrank(); } + function test04_DepositThenDelegate() public { + // since deposit and delegate are already tested, we will just do a simple success + // check here to ensure the reentrancy modifier works. + test03_RegisterOperator(); + vm.startPrank(addrs[0]); + IVault vault = IVault(bootstrap.tokenToVault(address(myToken))); + myToken.approve(address(vault), amounts[0]); + bootstrap.depositThenDelegateTo(address(myToken), amounts[0], "exo13hasr43vvq8v44xpzh0l6yuym4kca98f87j7ac"); + vm.stopPrank(); + + uint256 deposited = bootstrap.totalDepositAmounts(addrs[0], address(myToken)); + assertTrue(deposited == amounts[0]); + + uint256 delegated = bootstrap.delegations(addrs[0], "exo13hasr43vvq8v44xpzh0l6yuym4kca98f87j7ac", + address(myToken)); + assertTrue(delegated == amounts[0]); + } + function test05_ReplaceKey() public { IOperatorRegistry.Commission memory commission = IOperatorRegistry.Commission(0, 1e18, 1e18); // Register operator