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

@@ -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;

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

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);
@@ -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");
 

contracts/p1/Main.sol

@@ -6,6 +6,7 @@
 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 @@
  * @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 @@
         require(address(rsr_) != address(0), "invalid RSR address");
         __Auth_init(shortFreeze_, longFreeze_);
         __ComponentRegistry_init(components);
+        __ReentrancyGuard_init();
         __UUPSUpgradeable_init();
 
         rsr = rsr_;
@@ -149,13 +159,23 @@
         );
     }
 
+    // === 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");
     }
 
     function _upgradeProxy(address proxy, address implementation) internal {
         (bool success, ) = proxy.call(
             abi.encodeWithSelector(UUPSUpgradeable.upgradeTo.selector, implementation)
         );
 

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");
@@ -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;

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