4.1.0

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.

contracts/interfaces/IMain.sol

@@ -183,6 +183,12 @@ 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 {

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

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 {}
 }

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();
+    }
 }

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);
@@ -107,7 +107,7 @@ contract BackingManagerP1 is TradingP1, IBackingManager {
     /// @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 ==
@@ -179,7 +179,7 @@ contract BackingManagerP1 is TradingP1, IBackingManager {
     /// @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");
 

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,
+    IMain,
+    GlobalReentrancyGuard
+{
     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 {
+        _nonReentrantBefore();
+    }
+
+    function endTx() external virtual {
+        _nonReentrantAfter();
+    }
+
     // === Upgradeability ===
     function _authorizeUpgrade(address) internal view override {
         require(msg.sender == address(this), "not self");

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),

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");
@@ -108,7 +112,7 @@ contract RevenueTraderP1 is TradingP1, IRevenueTrader {
     //      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;

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();
 

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 {}
 

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;
+    }
+}

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,17 +64,17 @@ 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());
     }
 
     /// 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");