diff --git a/CHANGELOG.md b/CHANGELOG.md index f91b51e826..f6f6a349ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ # Changelog +# 4.1.0 + +This release implements a global lock on `Main` (by inherinting from `GlobalReentrancyGuard.sol`), which can be used by individual components to define the `globalNonReentrant` modifier and allow global reentrancy checks accross core protocol functions. See [docs/solidity-style.md](./docs/solidity-style.md#Reentrancy-safety) + +- Adds `mixins/GlobalReentrancyGuard.sol` contract +- Implements the `globalNonReentrant` modifier on `ComponentP1` +- Adds to `globalNonReentrant` modifier on impacted functions to enforce global reentrancy checks + # 4.0.0 This release prepares the core protocol for veRSR through the introduction of 3 registries (`DAOFeeRegistry`, `AssetPluginRegistry`, and `VersionRegistry`) and through restricting component upgrades to be handled by `Main`, where upgrade constraints can be enforced. diff --git a/contracts/interfaces/IMain.sol b/contracts/interfaces/IMain.sol index 3c87331391..876b4e8010 100644 --- a/contracts/interfaces/IMain.sol +++ b/contracts/interfaces/IMain.sol @@ -156,6 +156,8 @@ interface IComponentRegistry { event BrokerSet(IBroker oldVal, IBroker newVal); function broker() external view returns (IBroker); + + function isComponent(address addr) external view returns (bool); } /** @@ -183,9 +185,17 @@ interface IMain is IVersioned, IAuth, IComponentRegistry { function versionRegistry() external view returns (VersionRegistry); function daoFeeRegistry() external view returns (DAOFeeRegistry); + + // === Control flow === + + function beginTx() external; + + function endTx() external; } interface TestIMain is IMain { + error ReentrancyGuardReentrantCall(); + function setVersionRegistry(VersionRegistry) external; function setAssetPluginRegistry(AssetPluginRegistry) external; diff --git a/contracts/mixins/ComponentRegistry.sol b/contracts/mixins/ComponentRegistry.sol index ff3a29f7c7..0bafd742c4 100644 --- a/contracts/mixins/ComponentRegistry.sol +++ b/contracts/mixins/ComponentRegistry.sol @@ -35,6 +35,7 @@ abstract contract ComponentRegistry is Initializable, Auth, IComponentRegistry { function _setRToken(IRToken val) private { require(address(val) != address(0), "invalid RToken address"); emit RTokenSet(rToken, val); + isComponent[address(val)] = true; rToken = val; } @@ -43,6 +44,7 @@ abstract contract ComponentRegistry is Initializable, Auth, IComponentRegistry { function _setStRSR(IStRSR val) private { require(address(val) != address(0), "invalid StRSR address"); emit StRSRSet(stRSR, val); + isComponent[address(val)] = true; stRSR = val; } @@ -51,6 +53,7 @@ abstract contract ComponentRegistry is Initializable, Auth, IComponentRegistry { function _setAssetRegistry(IAssetRegistry val) private { require(address(val) != address(0), "invalid AssetRegistry address"); emit AssetRegistrySet(assetRegistry, val); + isComponent[address(val)] = true; assetRegistry = val; } @@ -59,6 +62,7 @@ abstract contract ComponentRegistry is Initializable, Auth, IComponentRegistry { function _setBasketHandler(IBasketHandler val) private { require(address(val) != address(0), "invalid BasketHandler address"); emit BasketHandlerSet(basketHandler, val); + isComponent[address(val)] = true; basketHandler = val; } @@ -67,6 +71,7 @@ abstract contract ComponentRegistry is Initializable, Auth, IComponentRegistry { function _setBackingManager(IBackingManager val) private { require(address(val) != address(0), "invalid BackingManager address"); emit BackingManagerSet(backingManager, val); + isComponent[address(val)] = true; backingManager = val; } @@ -75,6 +80,7 @@ abstract contract ComponentRegistry is Initializable, Auth, IComponentRegistry { function _setDistributor(IDistributor val) private { require(address(val) != address(0), "invalid Distributor address"); emit DistributorSet(distributor, val); + isComponent[address(val)] = true; distributor = val; } @@ -83,6 +89,7 @@ abstract contract ComponentRegistry is Initializable, Auth, IComponentRegistry { function _setRSRTrader(IRevenueTrader val) private { require(address(val) != address(0), "invalid RSRTrader address"); emit RSRTraderSet(rsrTrader, val); + isComponent[address(val)] = true; rsrTrader = val; } @@ -91,6 +98,7 @@ abstract contract ComponentRegistry is Initializable, Auth, IComponentRegistry { function _setRTokenTrader(IRevenueTrader val) private { require(address(val) != address(0), "invalid RTokenTrader address"); emit RTokenTraderSet(rTokenTrader, val); + isComponent[address(val)] = true; rTokenTrader = val; } @@ -99,6 +107,7 @@ abstract contract ComponentRegistry is Initializable, Auth, IComponentRegistry { function _setFurnace(IFurnace val) private { require(address(val) != address(0), "invalid Furnace address"); emit FurnaceSet(furnace, val); + isComponent[address(val)] = true; furnace = val; } @@ -107,13 +116,35 @@ abstract contract ComponentRegistry is Initializable, Auth, IComponentRegistry { function _setBroker(IBroker val) private { require(address(val) != address(0), "invalid Broker address"); emit BrokerSet(broker, val); + isComponent[address(val)] = true; broker = val; } + // 4.1.0 - Required for global lock + mapping(address => bool) public isComponent; + + modifier onlyComponent() { + require(isComponent[_msgSender()], "not a component"); + _; + } + + function cacheComponents() external { + isComponent[address(rToken)] = true; + isComponent[address(stRSR)] = true; + isComponent[address(assetRegistry)] = true; + isComponent[address(basketHandler)] = true; + isComponent[address(backingManager)] = true; + isComponent[address(distributor)] = true; + isComponent[address(rsrTrader)] = true; + isComponent[address(rTokenTrader)] = true; + isComponent[address(furnace)] = true; + isComponent[address(broker)] = true; + } + /** * @dev This empty reserved space is put in place to allow future versions to add new * variables without shifting down storage in the inheritance chain. * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps */ - uint256[40] private __gap; + uint256[39] private __gap; } diff --git a/contracts/mixins/Versioned.sol b/contracts/mixins/Versioned.sol index 3c0038087e..cfd74d99ee 100644 --- a/contracts/mixins/Versioned.sol +++ b/contracts/mixins/Versioned.sol @@ -4,7 +4,7 @@ pragma solidity 0.8.19; import "../interfaces/IVersioned.sol"; // This value should be updated on each release -string constant VERSION = "4.0.0"; +string constant VERSION = "4.1.0"; /** * @title Versioned diff --git a/contracts/p0/Main.sol b/contracts/p0/Main.sol index 856df61362..3948d51f83 100644 --- a/contracts/p0/Main.sol +++ b/contracts/p0/Main.sol @@ -59,4 +59,12 @@ contract MainP0 is Versioned, Initializable, Auth, ComponentRegistry, IMain { function daoFeeRegistry() external pure returns (DAOFeeRegistry) { return DAOFeeRegistry(address(0)); } + + // === Control flow === + + // solhint-disable-next-line no-empty-blocks + function beginTx() external virtual {} + + // solhint-disable-next-line no-empty-blocks + function endTx() external virtual {} } diff --git a/contracts/p0/mixins/Component.sol b/contracts/p0/mixins/Component.sol index c1e64d4a22..7e62372ade 100644 --- a/contracts/p0/mixins/Component.sol +++ b/contracts/p0/mixins/Component.sol @@ -44,4 +44,13 @@ abstract contract ComponentP0 is Versioned, Initializable, ContextUpgradeable, I require(main.hasRole(OWNER, _msgSender()), "governance only"); _; } + + // === Control Flow === + // In P0 we do not apply locks + + modifier globalNonReentrant() { + main.beginTx(); + _; + main.endTx(); + } } diff --git a/contracts/p1/BackingManager.sol b/contracts/p1/BackingManager.sol index 2129eab875..fec8af261c 100644 --- a/contracts/p1/BackingManager.sol +++ b/contracts/p1/BackingManager.sol @@ -66,7 +66,7 @@ contract BackingManagerP1 is TradingP1, IBackingManager { // checks: erc20 in assetRegistry // action: set allowance on erc20 for rToken to UINT_MAX // Using two safeApprove calls instead of safeIncreaseAllowance to support USDT - function grantRTokenAllowance(IERC20 erc20) external notFrozen { + function grantRTokenAllowance(IERC20 erc20) external notFrozen globalNonReentrant { require(assetRegistry.isRegistered(erc20), "erc20 unregistered"); // == Interaction == IERC20(address(erc20)).safeApprove(address(rToken), 0); @@ -105,9 +105,7 @@ contract BackingManagerP1 is TradingP1, IBackingManager { /// Apply the overall backing policy using the specified TradeKind, taking a haircut if unable /// @param kind TradeKind.DUTCH_AUCTION or TradeKind.BATCH_AUCTION /// @custom:interaction not RCEI; nonReentrant - // untested: - // OZ nonReentrant line is assumed to be working. cost/benefit of direct testing is high - function rebalance(TradeKind kind) external nonReentrant { + function rebalance(TradeKind kind) external globalNonReentrant { requireNotTradingPausedOrFrozen(); // == Refresh == @@ -177,9 +175,7 @@ contract BackingManagerP1 is TradingP1, IBackingManager { /// Forward revenue to RevenueTraders; reverts if not fully collateralized /// @param erc20s The tokens to forward /// @custom:interaction not RCEI; nonReentrant - // untested: - // OZ nonReentrant line is assumed to be working. cost/benefit of direct testing is high - function forwardRevenue(IERC20[] calldata erc20s) external nonReentrant { + function forwardRevenue(IERC20[] calldata erc20s) external globalNonReentrant { requireNotTradingPausedOrFrozen(); require(ArrayLib.allUnique(erc20s), "duplicate tokens"); diff --git a/contracts/p1/Main.sol b/contracts/p1/Main.sol index e3cbec87da..14a2a542da 100644 --- a/contracts/p1/Main.sol +++ b/contracts/p1/Main.sol @@ -6,6 +6,7 @@ import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import "../interfaces/IMain.sol"; +import "./mixins/GlobalReentrancyGuard.sol"; import "../mixins/ComponentRegistry.sol"; import "../mixins/Auth.sol"; import "../mixins/Versioned.sol"; @@ -19,7 +20,15 @@ import "../interfaces/IBroker.sol"; * @notice The center of the system around which Components orbit. */ // solhint-disable max-states-count -contract MainP1 is Versioned, Initializable, Auth, ComponentRegistry, UUPSUpgradeable, IMain { +contract MainP1 is + Versioned, + Initializable, + Auth, + ComponentRegistry, + UUPSUpgradeable, + GlobalReentrancyGuard, + IMain +{ IERC20 public rsr; VersionRegistry public versionRegistry; AssetPluginRegistry public assetPluginRegistry; @@ -39,6 +48,7 @@ contract MainP1 is Versioned, Initializable, Auth, ComponentRegistry, UUPSUpgrad require(address(rsr_) != address(0), "invalid RSR address"); __Auth_init(shortFreeze_, longFreeze_); __ComponentRegistry_init(components); + __ReentrancyGuard_init(); __UUPSUpgradeable_init(); rsr = rsr_; @@ -149,6 +159,16 @@ contract MainP1 is Versioned, Initializable, Auth, ComponentRegistry, UUPSUpgrad ); } + // === Control Flow === + + function beginTx() external virtual onlyComponent { + _nonReentrantBefore(); + } + + function endTx() external virtual onlyComponent { + _nonReentrantAfter(); + } + // === Upgradeability === function _authorizeUpgrade(address) internal view override { require(msg.sender == address(this), "not self"); diff --git a/contracts/p1/RToken.sol b/contracts/p1/RToken.sol index 1c81c3b411..bcf4150cc9 100644 --- a/contracts/p1/RToken.sol +++ b/contracts/p1/RToken.sol @@ -102,7 +102,11 @@ contract RTokenP1 is ComponentP1, ERC20PermitUpgradeable, IRToken { /// @param amount {qRTok} The quantity of RToken to issue /// @custom:interaction RCEI // BU exchange rate cannot decrease, and it can only increase when < FIX_ONE. - function issueTo(address recipient, uint256 amount) public notIssuancePausedOrFrozen { + function issueTo(address recipient, uint256 amount) + public + notIssuancePausedOrFrozen + globalNonReentrant + { require(amount != 0, "Cannot issue zero"); // == Refresh == @@ -180,7 +184,7 @@ contract RTokenP1 is ComponentP1, ERC20PermitUpgradeable, IRToken { /// @param recipient The address to receive the backing collateral tokens /// @param amount {qRTok} The quantity {qRToken} of RToken to redeem /// @custom:interaction RCEI - function redeemTo(address recipient, uint256 amount) public notFrozen { + function redeemTo(address recipient, uint256 amount) public notFrozen globalNonReentrant { // == Refresh == assetRegistry.refresh(); @@ -258,7 +262,7 @@ contract RTokenP1 is ComponentP1, ERC20PermitUpgradeable, IRToken { uint192[] memory portions, address[] memory expectedERC20sOut, uint256[] memory minAmounts - ) external notFrozen { + ) external notFrozen globalNonReentrant { // == Refresh == assetRegistry.refresh(); @@ -427,7 +431,7 @@ contract RTokenP1 is ComponentP1, ERC20PermitUpgradeable, IRToken { /// Sends all token balance of erc20 (if it is registered) to the BackingManager /// @custom:interaction - function monetizeDonations(IERC20 erc20) external notTradingPausedOrFrozen { + function monetizeDonations(IERC20 erc20) external notTradingPausedOrFrozen globalNonReentrant { require(assetRegistry.isRegistered(erc20), "erc20 unregistered"); IERC20Upgradeable(address(erc20)).safeTransfer( address(backingManager), diff --git a/contracts/p1/RevenueTrader.sol b/contracts/p1/RevenueTrader.sol index fe0d24a50e..6ddc8091a9 100644 --- a/contracts/p1/RevenueTrader.sol +++ b/contracts/p1/RevenueTrader.sol @@ -67,13 +67,17 @@ contract RevenueTraderP1 is TradingP1, IRevenueTrader { /// Distribute tokenToBuy to its destinations /// @dev Special-case of manageTokens([tokenToBuy], *) /// @custom:interaction - function distributeTokenToBuy() external notTradingPausedOrFrozen { + function distributeTokenToBuy() external notTradingPausedOrFrozen globalNonReentrant { _distributeTokenToBuy(); } /// Return registered ERC20s to the BackingManager if distribution for tokenToBuy is 0 /// @custom:interaction - function returnTokens(IERC20[] memory erc20s) external notTradingPausedOrFrozen { + function returnTokens(IERC20[] memory erc20s) + external + notTradingPausedOrFrozen + globalNonReentrant + { RevenueTotals memory revTotals = distributor.totals(); if (tokenToBuy == rsr) { require(revTotals.rsrTotal == 0, "rsrTotal > 0"); @@ -104,11 +108,9 @@ contract RevenueTraderP1 is TradingP1, IRevenueTrader { // For each ERC20: // if erc20 is tokenToBuy: distribute it // else: sell erc20 for tokenToBuy - // untested: - // OZ nonReentrant line is assumed to be working. cost/benefit of direct testing is high function manageTokens(IERC20[] calldata erc20s, TradeKind[] calldata kinds) external - nonReentrant + globalNonReentrant notTradingPausedOrFrozen { uint256 len = erc20s.length; diff --git a/contracts/p1/StRSR.sol b/contracts/p1/StRSR.sol index 190aeb6258..9e79af56cb 100644 --- a/contracts/p1/StRSR.sol +++ b/contracts/p1/StRSR.sol @@ -226,7 +226,7 @@ abstract contract StRSRP1 is Initializable, ComponentP1, IStRSR, EIP712Upgradeab // // actions: // rsr.transferFrom(account, this, rsrAmount) - function stake(uint256 rsrAmount) public { + function stake(uint256 rsrAmount) public globalNonReentrant { _notZero(rsrAmount); _payoutRewards(); @@ -258,7 +258,7 @@ abstract contract StRSRP1 is Initializable, ComponentP1, IStRSR, EIP712Upgradeab // // A draft for (totalDrafts' - totalDrafts) drafts // is freshly appended to the caller's draft record. - function unstake(uint256 stakeAmount) external { + function unstake(uint256 stakeAmount) external globalNonReentrant { _requireNotTradingPausedOrFrozen(); _notZero(stakeAmount); @@ -303,7 +303,7 @@ abstract contract StRSRP1 is Initializable, ComponentP1, IStRSR, EIP712Upgradeab // // actions: // rsr.transfer(account, rsrOut) - function withdraw(address account, uint256 endId) external { + function withdraw(address account, uint256 endId) external globalNonReentrant { _requireNotTradingPausedOrFrozen(); uint256 firstId = firstRemainingDraft[draftEra][account]; @@ -345,7 +345,7 @@ abstract contract StRSRP1 is Initializable, ComponentP1, IStRSR, EIP712Upgradeab /// Cancel an ongoing unstaking; resume staking /// @custom:interaction CEI - function cancelUnstake(uint256 endId) external { + function cancelUnstake(uint256 endId) external globalNonReentrant { _requireNotFrozen(); address account = _msgSender(); diff --git a/contracts/p1/mixins/Component.sol b/contracts/p1/mixins/Component.sol index 9fc44c25d5..bebe5ada7e 100644 --- a/contracts/p1/mixins/Component.sol +++ b/contracts/p1/mixins/Component.sol @@ -63,6 +63,19 @@ abstract contract ComponentP1 is _; } + // === Control Flow === + + /** + * @dev Prevents reentrancy by implementing a global lock shared by all components + * Calling a `globalNonReentrant` function from another `globalNonReentrant` + * function is not supported. + */ + modifier globalNonReentrant() { + main.beginTx(); + _; + main.endTx(); + } + // solhint-disable-next-line no-empty-blocks function _authorizeUpgrade(address newImplementation) internal view override onlyMain {} diff --git a/contracts/p1/mixins/GlobalReentrancyGuard.sol b/contracts/p1/mixins/GlobalReentrancyGuard.sol new file mode 100644 index 0000000000..9333cb0401 --- /dev/null +++ b/contracts/p1/mixins/GlobalReentrancyGuard.sol @@ -0,0 +1,89 @@ +// SPDX-License-Identifier: BlueOak-1.0.0 +pragma solidity 0.8.19; + +// solhint-disable-next-line max-line-length +// Based on OpenZeppelin Upgradeable Contracts (last updated v5.1.0) (utils/ReentrancyGuardUpgradeable.sol) + +import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; + +/** + * @dev Contract module that helps prevent global reentrant calls. + * + * Inheriting from `GlobalReentrancyGuard` will allow to implement a global lock + * and allow other contracts to define the {globalNonReentrant} modifier, + * which can be applied to functions to make sure there are no nested (reentrant) + * calls among them. + * + */ +abstract contract GlobalReentrancyGuard is Initializable { + // Booleans are more expensive than uint256 or any type that takes up a full + // word because each write operation emits an extra SLOAD to first read the + // slot's contents, replace the bits taken up by the boolean, and then write + // back. This is the compiler's defense against contract upgrades and + // pointer aliasing, and it cannot be disabled. + + // The values being non-zero value makes deployment a bit more expensive, + // but in exchange the refund on every call to nonReentrant will be lower in + // amount. Since refunds are capped to a percentage of the total + // transaction's gas, it is best to keep them low in cases like this one, to + // increase the likelihood of the full refund coming into effect. + uint256 private constant NOT_ENTERED = 1; + uint256 private constant ENTERED = 2; + + /// @custom:storage-location erc7201:openzeppelin.storage.ReentrancyGuard + struct ReentrancyGuardStorage { + uint256 _status; + } + + // solhint-disable-next-line max-line-length + // keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.ReentrancyGuard")) - 1)) & ~bytes32(uint256(0xff)) + bytes32 private constant ReentrancyGuardStorageLocation = + 0x9b779b17422d0df92223018b32b4d1fa46e071723d6817e2486d003becc55f00; + + function _getReentrancyGuardStorage() private pure returns (ReentrancyGuardStorage storage $) { + assembly { + $.slot := ReentrancyGuardStorageLocation + } + } + + /** + * @dev Unauthorized reentrant call. + */ + error ReentrancyGuardReentrantCall(); + + function __ReentrancyGuard_init() internal onlyInitializing { + __ReentrancyGuard_init_unchained(); + } + + function __ReentrancyGuard_init_unchained() internal onlyInitializing { + ReentrancyGuardStorage storage $ = _getReentrancyGuardStorage(); + $._status = NOT_ENTERED; + } + + function _nonReentrantBefore() internal { + ReentrancyGuardStorage storage $ = _getReentrancyGuardStorage(); + // On the first call to nonReentrant, _status will be NOT_ENTERED + if ($._status == ENTERED) { + revert ReentrancyGuardReentrantCall(); + } + + // Any calls to nonReentrant after this point will fail + $._status = ENTERED; + } + + function _nonReentrantAfter() internal { + ReentrancyGuardStorage storage $ = _getReentrancyGuardStorage(); + // By storing the original value once again, a refund is triggered (see + // https://eips.ethereum.org/EIPS/eip-2200) + $._status = NOT_ENTERED; + } + + /** + * @dev Returns true if the reentrancy guard is currently set to "entered", which indicates + * there is a `nonReentrant` function in the call stack. + */ + function _reentrancyGuardEntered() internal view returns (bool) { + ReentrancyGuardStorage storage $ = _getReentrancyGuardStorage(); + return $._status == ENTERED; + } +} diff --git a/contracts/p1/mixins/Trading.sol b/contracts/p1/mixins/Trading.sol index 49c06a5810..de54d5cf68 100644 --- a/contracts/p1/mixins/Trading.sol +++ b/contracts/p1/mixins/Trading.sol @@ -16,6 +16,9 @@ import "./RewardableLib.sol"; /// changed without breaking <3.0.0 RTokens. The only difference in /// MulticallUpgradeable is the 50 slot storage gap and an empty constructor. /// It should be fine to leave the non-upgradeable Multicall here permanently. +/// Note: For >=4.1.0 RTokens, ReentrancyGuardUpgradeable is deprecated and +/// replaced with GlobalReentrancyGuard (`globalNonReentrant`), but +/// kept as base contract for storage slot compatibility. abstract contract TradingP1 is Multicall, ComponentP1, ReentrancyGuardUpgradeable, ITrading { using FixLib for uint192; @@ -61,8 +64,8 @@ abstract contract TradingP1 is Multicall, ComponentP1, ReentrancyGuardUpgradeabl /// Claim all rewards /// Collective Action - /// @custom:interaction CEI - function claimRewards() external { + /// @custom:interaction CEI (marked `nonReentrant`) + function claimRewards() external globalNonReentrant { requireNotTradingPausedOrFrozen(); RewardableLibP1.claimRewards(main.assetRegistry()); } @@ -70,8 +73,8 @@ abstract contract TradingP1 is Multicall, ComponentP1, ReentrancyGuardUpgradeabl /// Claim rewards for a single asset /// Collective Action /// @param erc20 The ERC20 to claimRewards on - /// @custom:interaction CEI - function claimRewardsSingle(IERC20 erc20) external { + /// @custom:interaction CEI (marked `nonReentrant`) + function claimRewardsSingle(IERC20 erc20) external globalNonReentrant { requireNotTradingPausedOrFrozen(); RewardableLibP1.claimRewardsSingle(main.assetRegistry().toAsset(erc20)); } @@ -89,9 +92,7 @@ abstract contract TradingP1 is Multicall, ComponentP1, ReentrancyGuardUpgradeabl // effects: // trades.set(sell, 0) // tradesOpen' = tradesOpen - 1 - // untested: - // OZ nonReentrant line is assumed to be working. cost/benefit of direct testing is high - function settleTrade(IERC20 sell) public virtual nonReentrant returns (ITrade trade) { + function settleTrade(IERC20 sell) public virtual globalNonReentrant returns (ITrade trade) { trade = trades[sell]; require(address(trade) != address(0), "no trade open"); require(trade.canSettle(), "cannot settle yet"); diff --git a/contracts/plugins/assets/VersionedAsset.sol b/contracts/plugins/assets/VersionedAsset.sol index 9d5b828b3a..12bc79a822 100644 --- a/contracts/plugins/assets/VersionedAsset.sol +++ b/contracts/plugins/assets/VersionedAsset.sol @@ -4,7 +4,7 @@ pragma solidity 0.8.19; import "../../interfaces/IVersioned.sol"; // This value should be updated on each release -string constant ASSET_VERSION = "4.0.0"; +string constant ASSET_VERSION = "4.1.0"; /** * @title VersionedAsset diff --git a/contracts/plugins/mocks/ERC20MockReentrant.sol b/contracts/plugins/mocks/ERC20MockReentrant.sol new file mode 100644 index 0000000000..1a61c74966 --- /dev/null +++ b/contracts/plugins/mocks/ERC20MockReentrant.sol @@ -0,0 +1,55 @@ +// SPDX-License-Identifier: BlueOak-1.0.0 +pragma solidity 0.8.19; + +import "@openzeppelin/contracts/utils/Address.sol"; +import "./ERC20Mock.sol"; +import "../../interfaces/IMain.sol"; + +contract ERC20MockReentrant is ERC20Mock { + bool private _reenter; + address private _reentryTarget; + bytes private _reentryCall; + + constructor(string memory name, string memory symbol) ERC20Mock(name, symbol) { + _reenter = false; + } + + function setReenter(bool value) external { + _reenter = value; + } + + function setReentryCall(address target, bytes calldata call) external { + _reentryTarget = target; + _reentryCall = call; + } + + function transferFrom( + address from, + address to, + uint256 amount + ) public override returns (bool) { + _reentrancy(); + return super.transferFrom(from, to, amount); + } + + function transfer(address to, uint256 amount) public override returns (bool) { + _reentrancy(); + return super.transfer(to, amount); + } + + function approve(address spender, uint256 amount) public override returns (bool) { + _reentrancy(); + return super.approve(spender, amount); + } + + // Mock function only used for testing claimRewards + function claimRewards() public { + _reentrancy(); + } + + function _reentrancy() private { + if (_reenter && _reentryCall.length > 0 && _reentryTarget != address(0)) { + Address.functionCall(_reentryTarget, _reentryCall); // bubble revert + } + } +} diff --git a/contracts/plugins/mocks/FiatCollateralMockReentrant.sol b/contracts/plugins/mocks/FiatCollateralMockReentrant.sol new file mode 100644 index 0000000000..20d2db9d01 --- /dev/null +++ b/contracts/plugins/mocks/FiatCollateralMockReentrant.sol @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: BlueOak-1.0.0 +pragma solidity 0.8.19; + +import "../assets/FiatCollateral.sol"; +import "./ERC20MockReentrant.sol"; + +// Use with ERC20MockReentrant.sol for reentrancy tests +contract FiatCollateralMockReentrant is FiatCollateral { + constructor(CollateralConfig memory config) FiatCollateral(config) {} + + function claimRewards() external override(Asset, IRewardable) { + ERC20MockReentrant(address(erc20)).claimRewards(); // force reentrancy + } +} diff --git a/docs/solidity-style.md b/docs/solidity-style.md index 35bbf87698..e5721251d4 100644 --- a/docs/solidity-style.md +++ b/docs/solidity-style.md @@ -161,7 +161,7 @@ The protocol functions best when whole RSR is worth `>= $0.001`. This constraint All core functions that can be called from outside our system are classified into one of the following 3 categories: -1. `@custom:interaction` - An action. Disallowed while paused. Per-contract reentrancy-safety is needed. +1. `@custom:interaction` - An action. Disallowed while paused. Global reentrancy-safety is needed. 2. `@custom:governance` - Governance change. Allowed while paused. 3. `@custom:refresher` - Non-system-critical state transitions. Disallowed while paused, with the exception of `refresh()`. @@ -179,12 +179,17 @@ For each `external` or `public` function, one of these tags MUST be in the corre - stRSR.withdraw() - rToken.issue() - rToken.redeem() -- {rsrTrader,rTokenTrader,backingManager}.claimRewards() +- rToken.redeemTo() +- rToken.redeemCustom() +- rToken.monetizeDonations() +- {rsrTrader,rTokenTrader,backingManager}.claimRewards() / .claimRewardsSingle() - {rsrTrader,rTokenTrader,backingManager}.settleTrade() - backingManager.grantRTokenAllowances() -- backingManager.rebalance\*() -- backingManager.forwardRevenue\*() +- backingManager.rebalance() +- backingManager.forwardRevenue() - {rsrTrader,rTokenTrader}.manageTokens() +- {rsrTrader,rTokenTrader}.distributeTokenToBuy() +- {rsrTrader,rTokenTrader}.returnTokens() ### `@custom:governance` @@ -236,9 +241,9 @@ At the start of the Interactions block in a CEI-pattern function, set them off v When a function is an interaction made reentrancy-safe by the CEI pattern, follow its `@custom:interaction` mark with `CEI`, or with `RCEI` (R is for "Refresh") if it starts by calling `AssetRegistry.refresh()`. -#### ReentrancyGuard +#### ReentrancyGuard (Global Lock) -Where using the CEI pattern is impractical, every function on that contract that is `external`, and can write to the relevant state elements, should use `reentrancyGuard`. That is, the contract should inherit from either `ReentrancyGuard` (or `ReentrancyGuardUpgradable` as needed), and every external function that can either modify contract state, or read it when it's inconsistent, should be marked with the `nonReentrant` modifier. +Where using the CEI pattern is impractical, every function that is `external`, and can write to the relevant state elements, should use a global lock implemented via `GlobalReentrancyGuard` on `Main`. Every `@custom:interaction` should be market with the `globalNonReentrant` modifier. #### Exceptions diff --git a/test/Main.test.ts b/test/Main.test.ts index d342e0e79e..49ff65dffa 100644 --- a/test/Main.test.ts +++ b/test/Main.test.ts @@ -18,6 +18,7 @@ import { import { CollateralStatus, RoundingMode, + TradeKind, ZERO_ADDRESS, ONE_ADDRESS, MAX_UINT256, @@ -26,6 +27,8 @@ import { LONG_FREEZER, PAUSER, MAX_UINT192, + FURNACE_DEST, + STRSR_DEST, } from '../common/constants' import { expectEqualArrays } from './utils/matchers' import { expectInIndirectReceipt, expectInReceipt, expectEvents } from '../common/events' @@ -40,8 +43,10 @@ import { DutchTrade, CTokenMock, ERC20Mock, + ERC20MockReentrant, FacadeTest, FiatCollateral, + FiatCollateralMockReentrant, GnosisMock, GnosisTrade, IAssetRegistry, @@ -254,6 +259,18 @@ describe(`MainP${IMPLEMENTATION} contract`, () => { expect(await main.rsrTrader()).to.equal(rsrTrader.address) expect(await main.rTokenTrader()).to.equal(rTokenTrader.address) + // Components registered + expect(await main.isComponent(rToken.address)).to.equal(true) + expect(await main.isComponent(stRSR.address)).to.equal(true) + expect(await main.isComponent(assetRegistry.address)).to.equal(true) + expect(await main.isComponent(basketHandler.address)).to.equal(true) + expect(await main.isComponent(backingManager.address)).to.equal(true) + expect(await main.isComponent(distributor.address)).to.equal(true) + expect(await main.isComponent(rsrTrader.address)).to.equal(true) + expect(await main.isComponent(rTokenTrader.address)).to.equal(true) + expect(await main.isComponent(furnace.address)).to.equal(true) + expect(await main.isComponent(broker.address)).to.equal(true) + // Configuration const [rTokenTotal, rsrTotal] = await distributor.totals() expect(rTokenTotal).to.equal(bn(4000)) @@ -3678,6 +3695,367 @@ describe(`MainP${IMPLEMENTATION} contract`, () => { }) }) + describeP1('Global Lock (Non-reentrancy)', () => { + const issueAmount = fp('10000') + const amount: BigNumber = fp('10') + let daiChainlink: MockV3Aggregator + let reentrantToken: ERC20MockReentrant + let reentrantColl: FiatCollateral + let reentryCalls: Array<{ target: string; calldata: string }> + + beforeEach(async () => { + daiChainlink = await ethers.getContractAt( + 'MockV3Aggregator', + await collateral0.chainlinkFeed() + ) + + // Setup reentrant token/collateral + const ERC20ReentrantFactory: ContractFactory = await ethers.getContractFactory( + 'ERC20MockReentrant' + ) + const CollReentrantFactory: ContractFactory = await ethers.getContractFactory( + 'FiatCollateralMockReentrant' + ) + + reentrantToken = ( + await ERC20ReentrantFactory.deploy('Reentrant Token', 'ReentrantTKN') + ) + reentrantColl = await CollReentrantFactory.deploy({ + priceTimeout: PRICE_TIMEOUT, + chainlinkFeed: daiChainlink.address, + oracleError: ORACLE_ERROR, + erc20: reentrantToken.address, + maxTradeVolume: config.rTokenMaxTradeVolume, + oracleTimeout: ORACLE_TIMEOUT, + targetName: await ethers.utils.formatBytes32String('USD'), + defaultThreshold: DEFAULT_THRESHOLD, + delayUntilDefault: await collateral0.delayUntilDefault(), + }) + await assetRegistry.connect(owner).register(reentrantColl.address) + await reentrantToken.mint(addr1.address, issueAmount.mul(2)) + + // Setup reentrant basket + await basketHandler + .connect(owner) + .forceSetPrimeBasket([token0.address, reentrantToken.address], [fp('0.5'), fp('0.5')]) + await basketHandler.refreshBasket() + await advanceTime(Number(config.warmupPeriod) + 1) + + // register backups + await assetRegistry.connect(owner).register(backupCollateral1.address) + await basketHandler + .connect(owner) + .setBackupConfig(ethers.utils.formatBytes32String('USD'), bn(1), [backupToken1.address]) + + // issue rTokens + await token0.connect(addr1).approve(rToken.address, issueAmount) + await reentrantToken.connect(addr1).approve(rToken.address, issueAmount) + await rToken.connect(addr1).issue(issueAmount) + + // Perform donation + await reentrantToken.connect(owner).mint(rToken.address, fp(100)) + + // Set revenue on BM and Traders + await reentrantToken.connect(owner).mint(backingManager.address, fp(100)) + await reentrantToken.connect(owner).mint(rsrTrader.address, fp(100)) + await reentrantToken.connect(owner).mint(rTokenTrader.address, fp(100)) + + // Set RSR distribution to zero + await distributor.setDistributions( + [FURNACE_DEST, STRSR_DEST], + [ + { rTokenDist: bn(10000), rsrDist: bn(0) }, + { rTokenDist: bn('0'), rsrDist: bn('0') }, + ] + ) + + // turn on reentrancy + await reentrantToken.setReenter(true) + + // Set reentrancy calls to test + reentryCalls = [ + { + target: backingManager.address, + calldata: backingManager.interface.encodeFunctionData('forwardRevenue', [ + [reentrantToken.address], + ]), + }, + { + target: backingManager.address, + calldata: backingManager.interface.encodeFunctionData('grantRTokenAllowance', [ + token0.address, + ]), + }, + { + target: backingManager.address, + calldata: backingManager.interface.encodeFunctionData('rebalance', [ + TradeKind.DUTCH_AUCTION, + ]), + }, + { + target: backingManager.address, + calldata: backingManager.interface.encodeFunctionData('claimRewards'), + }, + { + target: backingManager.address, + calldata: backingManager.interface.encodeFunctionData('claimRewardsSingle', [ + token0.address, + ]), + }, + { + target: backingManager.address, + calldata: backingManager.interface.encodeFunctionData('settleTrade', [token0.address]), + }, + { + target: stRSR.address, + calldata: stRSR.interface.encodeFunctionData('stake', [amount]), + }, + { + target: stRSR.address, + calldata: stRSR.interface.encodeFunctionData('unstake', [amount]), + }, + { + target: stRSR.address, + calldata: stRSR.interface.encodeFunctionData('cancelUnstake', [0]), + }, + { + target: stRSR.address, + calldata: stRSR.interface.encodeFunctionData('withdraw', [addr1.address, 0]), + }, + { + target: rToken.address, + calldata: rToken.interface.encodeFunctionData('issue', [amount]), + }, + { + target: rToken.address, + calldata: rToken.interface.encodeFunctionData('redeem', [amount]), + }, + { + target: rToken.address, + calldata: rToken.interface.encodeFunctionData('redeemCustom', [ + addr1.address, + amount, + [], + [], + [], + [], + ]), + }, + { + target: rToken.address, + calldata: rToken.interface.encodeFunctionData('monetizeDonations', [token0.address]), + }, + { + target: rsrTrader.address, + calldata: rsrTrader.interface.encodeFunctionData('manageTokens', [ + [token0.address], + [TradeKind.DUTCH_AUCTION], + ]), + }, + { + target: rsrTrader.address, + calldata: rsrTrader.interface.encodeFunctionData('distributeTokenToBuy'), + }, + { + target: rsrTrader.address, + calldata: rsrTrader.interface.encodeFunctionData('returnTokens', [[token0.address]]), + }, + { + target: rTokenTrader.address, + calldata: rTokenTrader.interface.encodeFunctionData('claimRewards'), + }, + { + target: rTokenTrader.address, + calldata: rTokenTrader.interface.encodeFunctionData('claimRewardsSingle', [ + token0.address, + ]), + }, + { + target: rTokenTrader.address, + calldata: rTokenTrader.interface.encodeFunctionData('settleTrade', [token0.address]), + }, + ] + }) + + it('Should prevent reentrancy - Basic Ops', async () => { + const redeemAmount: BigNumber = fp('10000') + + // Enable reentrancy calls + await reentrantToken.setReenter(true) + + // Attempt reentrant calls + for (const { target, calldata } of reentryCalls) { + await reentrantToken.setReentryCall(target, calldata) + + // Redeem + await expect(rToken.connect(addr1).redeem(redeemAmount)).to.be.revertedWithCustomError( + main, + 'ReentrancyGuardReentrantCall' + ) + + // Custom Redeem + const basketNonces = [2] + const portions = [fp('1')] + const quote = await basketHandler.quoteCustomRedemption( + basketNonces, + portions, + redeemAmount + ) + await expect( + rToken + .connect(addr1) + .redeemCustom( + addr1.address, + redeemAmount, + basketNonces, + portions, + quote.erc20s, + quote.quantities + ) + ).to.be.revertedWithCustomError(main, 'ReentrancyGuardReentrantCall') + + // Issue + await token0.connect(addr1).approve(rToken.address, issueAmount) + await reentrantToken.setReenter(false) + await reentrantToken.connect(addr1).approve(rToken.address, issueAmount) + await reentrantToken.setReenter(true) + + await expect(rToken.connect(addr1).issue(issueAmount)).to.be.revertedWithCustomError( + main, + 'ReentrancyGuardReentrantCall' + ) + + // Monetize donations + await expect( + rToken.monetizeDonations(reentrantToken.address) + ).to.be.revertedWithCustomError(main, 'ReentrancyGuardReentrantCall') + + // Grant RToken allowance + await expect( + backingManager.connect(addr1).grantRTokenAllowance(reentrantToken.address) + ).to.be.revertedWithCustomError(main, 'ReentrancyGuardReentrantCall') + + // Forward Revenue + await expect( + backingManager.forwardRevenue([reentrantToken.address]) + ).to.be.revertedWithCustomError(main, 'ReentrancyGuardReentrantCall') + + // Return Tokens + await expect( + rsrTrader.returnTokens([reentrantToken.address]) + ).to.be.revertedWithCustomError(main, 'ReentrancyGuardReentrantCall') + + // Manage Tokens + await expect( + rTokenTrader.manageTokens([reentrantToken.address], [TradeKind.DUTCH_AUCTION]) + ).to.be.revertedWithCustomError(main, 'ReentrancyGuardReentrantCall') + + // Claim Rewards Single + await expect( + rTokenTrader.claimRewardsSingle(reentrantToken.address) + ).to.be.revertedWithCustomError(main, 'ReentrancyGuardReentrantCall') + + await expect( + backingManager.claimRewardsSingle(reentrantToken.address) + ).to.be.revertedWithCustomError(main, 'ReentrancyGuardReentrantCall') + + // Claim Rewards + await expect(rTokenTrader.claimRewards()).to.be.revertedWithCustomError( + main, + 'ReentrancyGuardReentrantCall' + ) + + await expect(backingManager.claimRewards()).to.be.revertedWithCustomError( + main, + 'ReentrancyGuardReentrantCall' + ) + } + }) + + it('Should prevent reentrancy - Rebalance', async () => { + // Enable reentrancy calls + await reentrantToken.setReenter(true) + + // Switch basket, remove reentrant token + await basketHandler.connect(owner).forceSetPrimeBasket([token0.address], [fp('1')]) + await basketHandler.refreshBasket() + await advanceTime(Number(config.warmupPeriod) + 1) + + // Attempt reentrant calls + for (const { target, calldata } of reentryCalls) { + await reentrantToken.setReentryCall(target, calldata) + + // Rebalance + await expect( + backingManager.rebalance(TradeKind.DUTCH_AUCTION) + ).to.be.revertedWithCustomError(main, 'ReentrancyGuardReentrantCall') + } + }) + + it('Should prevent reentrancy - Settle Trade', async () => { + // Start revenue auction + await rTokenTrader.manageTokens([reentrantToken.address], [TradeKind.DUTCH_AUCTION]) + await advanceTime(config.dutchAuctionLength.add(100).toString()) + + // Enable reentrancy calls + await reentrantToken.setReenter(true) + + // Attempt reentrant calls + for (const { target, calldata } of reentryCalls) { + await reentrantToken.setReentryCall(target, calldata) + + // Settle trade + await expect( + rTokenTrader.settleTrade(reentrantToken.address) + ).to.be.revertedWithCustomError(main, 'ReentrancyGuardReentrantCall') + } + }) + + it('Should allow to cache components', async () => { + await (await ethers.getContractAt('MainP1', main.address)).cacheComponents() + + expect(await main.isComponent(rToken.address)).to.equal(true) + expect(await main.isComponent(stRSR.address)).to.equal(true) + expect(await main.isComponent(assetRegistry.address)).to.equal(true) + expect(await main.isComponent(basketHandler.address)).to.equal(true) + expect(await main.isComponent(backingManager.address)).to.equal(true) + expect(await main.isComponent(distributor.address)).to.equal(true) + expect(await main.isComponent(rsrTrader.address)).to.equal(true) + expect(await main.isComponent(rTokenTrader.address)).to.equal(true) + expect(await main.isComponent(furnace.address)).to.equal(true) + expect(await main.isComponent(broker.address)).to.equal(true) + }) + + it('Should only allow components to begin-end txs', async () => { + await expect(main.connect(owner).beginTx()).to.be.revertedWith('not a component') + await expect(main.connect(owner).endTx()).to.be.revertedWith('not a component') + await expect(main.connect(other).beginTx()).to.be.revertedWith('not a component') + await expect(main.connect(other).endTx()).to.be.revertedWith('not a component') + + // Try with components + const components: Parameters[0] = { + rToken: rToken.address, + stRSR: stRSR.address, + assetRegistry: assetRegistry.address, + basketHandler: basketHandler.address, + backingManager: backingManager.address, + distributor: distributor.address, + rsrTrader: rsrTrader.address, + rTokenTrader: rTokenTrader.address, + furnace: furnace.address, + broker: broker.address, + } + + // Loop through all components + for (const comp of Object.values(components)) { + await whileImpersonating(comp, async (compSigner) => { + await expect(main.connect(compSigner).beginTx()).to.not.be.reverted + await expect(main.connect(compSigner).endTx()).to.not.be.reverted + }) + } + }) + }) + describeGas('Gas Reporting', () => { it('Asset Registry - Refresh', async () => { // Basket handler can run refresh diff --git a/test/fixtures.ts b/test/fixtures.ts index e86875dc54..1dae1fddd4 100644 --- a/test/fixtures.ts +++ b/test/fixtures.ts @@ -95,7 +95,7 @@ export const ORACLE_ERROR = fp('0.01') // 1% oracle error export const REVENUE_HIDING = fp('0') // no revenue hiding by default; test individually // This will have to be updated on each release -export const VERSION = '4.0.0' +export const VERSION = '4.1.0' export type Collateral = | FiatCollateral diff --git a/test/integration/UpgradeToR4.test.ts b/test/integration/UpgradeToR4.test.ts index 66a0416e8c..2cdf5c41d7 100644 --- a/test/integration/UpgradeToR4.test.ts +++ b/test/integration/UpgradeToR4.test.ts @@ -24,7 +24,8 @@ const rTokensToTest: RTokenParams[] = [ }, ] -const v4VersionHash = '0x81ed76178093786cbe0cb79744f6e7ca3336fbb9fe7d1ddff1f0157b63e09813' +// 4.1.0 +const v4VersionHash = '0x2b64e9eb005edea481c3384a1a7394d55c9ec0c75304d5daa56aad0d184c7fc3' async function _confirmVersion(address: string, target: string) { const versionedTarget = await ethers.getContractAt('Versioned', address) @@ -32,7 +33,7 @@ async function _confirmVersion(address: string, target: string) { } // NOTE: This is an explicit test! -describe('Upgrade from 3.4.0 to 4.0.0 (Mainnet Fork)', () => { +describe('Upgrade from 3.4.0 to 4.1.0 (Mainnet Fork)', () => { let implementations: IImplementations let deployer: DeployerP1 let versionRegistry: VersionRegistry @@ -129,7 +130,7 @@ describe('Upgrade from 3.4.0 to 4.0.0 (Mainnet Fork)', () => { ) await whileImpersonating(hre, TimelockController.address, async (signer) => { - // Upgrade Main to 4.0.0's Main + // Upgrade Main to 4.1.0's Main await RTokenMain.connect(signer).upgradeTo(implementations.main) // Set registries @@ -168,7 +169,7 @@ describe('Upgrade from 3.4.0 to 4.0.0 (Mainnet Fork)', () => { ] for (let j = 0; j < targetsToVerify.length; j++) { - await _confirmVersion(targetsToVerify[j], '4.0.0') + await _confirmVersion(targetsToVerify[j], '4.1.0') } const broker = await ethers.getContractAt('BrokerP1', await RTokenMain.broker()) @@ -179,7 +180,7 @@ describe('Upgrade from 3.4.0 to 4.0.0 (Mainnet Fork)', () => { // So, let's upgrade the RToken _again_ to verify the process flow works. await whileImpersonating(hre, TimelockController.address, async (signer) => { - // Upgrade Main to 4.0.0's Main + // Upgrade Main to 4.1.0's Main await RTokenMain.connect(signer).upgradeMainTo(v4VersionHash) // Upgrade RToken diff --git a/test/integration/UpgradeToR4WithRegistries.test.ts b/test/integration/UpgradeToR4WithRegistries.test.ts index 61648d3215..e2905b6d8c 100644 --- a/test/integration/UpgradeToR4WithRegistries.test.ts +++ b/test/integration/UpgradeToR4WithRegistries.test.ts @@ -23,7 +23,8 @@ const rTokensToTest: RTokenParams[] = [ }, ] -const v4VersionHash = '0x81ed76178093786cbe0cb79744f6e7ca3336fbb9fe7d1ddff1f0157b63e09813' +// 4.1.0 +const v4VersionHash = '0x2b64e9eb005edea481c3384a1a7394d55c9ec0c75304d5daa56aad0d184c7fc3' const v2VersionHash = '0xb4bcb154e38601c389396fa918314da42d4626f13ef6d0ceb07e5f5d26b2fbc3' async function _confirmVersion(address: string, target: string) { @@ -32,7 +33,7 @@ async function _confirmVersion(address: string, target: string) { } // NOTE: This is an explicit test! -describe('Upgrade from 4.0.0 to New Version with all Registries Enabled', () => { +describe('Upgrade from 4.1.0 to New Version with all Registries Enabled', () => { let versionRegistry: VersionRegistry let assetPluginRegistry: AssetPluginRegistry let daoFeeRegistry: DAOFeeRegistry @@ -191,7 +192,7 @@ describe('Upgrade from 4.0.0 to New Version with all Registries Enabled', () => ) await whileImpersonating(hre, TimelockController.address, async (signer) => { - // Upgrade Main to 4.0.0's Main + // Upgrade Main to 4.1.0's Main await RTokenMain.connect(signer).upgradeTo(implementationsR4.main) // Set registries @@ -230,7 +231,7 @@ describe('Upgrade from 4.0.0 to New Version with all Registries Enabled', () => ] for (let j = 0; j < targetsToVerify.length; j++) { - await _confirmVersion(targetsToVerify[j], '4.0.0') + await _confirmVersion(targetsToVerify[j], '4.1.0') } const currentAssetRegistry = await RTokenAssetRegistry.getRegistry() @@ -243,7 +244,7 @@ describe('Upgrade from 4.0.0 to New Version with all Registries Enabled', () => // So, let's upgrade the RToken to a new version now. await whileImpersonating(hre, TimelockController.address, async (signer) => { - // Upgrade Main to 4.0.0's Main + // Upgrade Main to 4.1.0's Main await RTokenMain.connect(signer).upgradeMainTo(v2VersionHash) // Registry does not have assets yet. @@ -266,7 +267,7 @@ describe('Upgrade from 4.0.0 to New Version with all Registries Enabled', () => // Finish upgrade, with asset validation await whileImpersonating(hre, TimelockController.address, async (signer) => { - // Upgrade Main to 4.0.0's Main + // Upgrade Main to 4.1.0's Main await RTokenMain.connect(signer).upgradeMainTo(v2VersionHash) // Upgrade RToken, without validating assets