Merge pull request #1204 from reserve-protocol/c4-mitigations-2024-07

C4 Mitigations - 2024-07

.solhintignore

@@ -20,3 +20,4 @@ contracts/vendor
 contracts/plugins/assets/compound/vendor
 contracts/plugins/assets/curve/crv
 contracts/plugins/assets/curve/cvx
+contracts/plugins/governance/vendor

common/configuration.ts

@@ -157,7 +157,6 @@ interface INetworkConfig {
   COMPTROLLER?: string
   FLUX_FINANCE_COMPTROLLER?: string
   GNOSIS_EASY_AUCTION?: string
-  EASY_AUCTION_OWNER?: string
   MORPHO_AAVE_CONTROLLER?: string
   MORPHO_REWARDS_DISTRIBUTOR?: string
   MORPHO_AAVE_LENS?: string
@@ -295,8 +294,7 @@ export const networkConfig: { [key: string]: INetworkConfig } = {
     AAVE_DATA_PROVIDER: '0x057835Ad21a177dbdd3090bB1CAE03EaCF78Fc6d',
     FLUX_FINANCE_COMPTROLLER: '0x95Af143a021DF745bc78e845b54591C53a8B3A51',
     COMPTROLLER: '0x3d9819210A31b4961b30EF54bE2aeD79B9c9Cd3B',
-    GNOSIS_EASY_AUCTION: '0x0b7fFc1f4AD541A4Ed16b40D8c37f0929158D101',
-    EASY_AUCTION_OWNER: '0x0da0c3e52c977ed3cbc641ff02dd271c3ed55afe',
+    GNOSIS_EASY_AUCTION: '0x462302752B63Ee7c207459112CA8D38498Fb54f2', // our deployment
     MORPHO_AAVE_LENS: '0x507fA343d0A90786d86C7cd885f5C49263A91FF4',
     MORPHO_AAVE_CONTROLLER: '0x777777c9898D384F785Ee44Acfe945efDFf5f3E0',
     MORPHO_REWARDS_DISTRIBUTOR: '0x3b14e5c73e0a56d607a8688098326fd4b4292135',
@@ -396,7 +394,7 @@ export const networkConfig: { [key: string]: INetworkConfig } = {
     AAVE_DATA_PROVIDER: '0x057835Ad21a177dbdd3090bB1CAE03EaCF78Fc6d',
     FLUX_FINANCE_COMPTROLLER: '0x95Af143a021DF745bc78e845b54591C53a8B3A51',
     COMPTROLLER: '0x3d9819210A31b4961b30EF54bE2aeD79B9c9Cd3B',
-    GNOSIS_EASY_AUCTION: '0x0b7fFc1f4AD541A4Ed16b40D8c37f0929158D101',
+    GNOSIS_EASY_AUCTION: '0x462302752B63Ee7c207459112CA8D38498Fb54f2', // our deployment
     MORPHO_AAVE_LENS: '0x507fA343d0A90786d86C7cd885f5C49263A91FF4',
     MORPHO_AAVE_CONTROLLER: '0x777777c9898D384F785Ee44Acfe945efDFf5f3E0',
     MORPHO_REWARDS_DISTRIBUTOR: '0x3b14e5c73e0a56d607a8688098326fd4b4292135',
@@ -708,6 +706,7 @@ export interface IGovRoles {
 // System constants
 export const MAX_TRADE_SLIPPAGE = BigNumber.from(10).pow(18)
 export const MAX_BACKING_BUFFER = BigNumber.from(10).pow(18)
+export const MIN_TARGET_AMT = BigNumber.from(10).pow(12)
 export const MAX_TARGET_AMT = BigNumber.from(10).pow(21)
 export const MAX_RATIO = BigNumber.from(10).pow(14)
 export const MAX_TRADE_VOLUME = BigNumber.from(10).pow(48)

contracts/interfaces/IBroker.sol

@@ -34,7 +34,6 @@ struct TradeRequest {
  *   the continued proper functioning of trading platforms.
  */
 interface IBroker is IComponent {
-    event GnosisSet(IGnosis oldVal, IGnosis newVal);
     event BatchTradeImplementationSet(ITrade oldVal, ITrade newVal);
     event DutchTradeImplementationSet(ITrade oldVal, ITrade newVal);
     event BatchAuctionLengthSet(uint48 oldVal, uint48 newVal);
@@ -45,7 +44,6 @@ interface IBroker is IComponent {
     // Initialization
     function init(
         IMain main_,
-        IGnosis gnosis_,
         ITrade batchTradeImplemention_,
         uint48 batchAuctionLength_,
         ITrade dutchTradeImplemention_,
@@ -86,8 +84,6 @@ interface TestIBroker is IBroker {
 
     function dutchAuctionLength() external view returns (uint48);
 
-    function setGnosis(IGnosis newGnosis) external;
-
     function setBatchTradeImplementation(ITrade newTradeImplementation) external;
 
     function setBatchAuctionLength(uint48 newAuctionLength) external;

contracts/interfaces/IDeployer.sol

@@ -123,8 +123,6 @@ interface TestIDeployer is IDeployer {
 
     function rsr() external view returns (IERC20Metadata);
 
-    function gnosis() external view returns (IGnosis);
-
     function rsrAsset() external view returns (IAsset);
 
     function implementations() external view returns (Implementations memory);

contracts/interfaces/ITrading.sol

@@ -43,6 +43,12 @@ interface ITrading is IComponent, IRewardableComponent {
         uint256 buyAmount
     );
 
+    /// Forcibly settle a trade, losing all value
+    /// Should only be called in case of censorship
+    /// @param trade The trade address itself
+    /// @custom:governance
+    function forceSettleTrade(ITrade trade) external;
+
     /// Settle a single trade, expected to be used with multicall for efficient mass settlement
     /// @param sell The sell token in the trade
     /// @return The trade settled

contracts/libraries/Fixed.sol

@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: BlueOak-1.0.0
 // solhint-disable func-name-mixedcase func-visibility
 // slither-disable-start divide-before-multiply
-pragma solidity ^0.8.19;
+pragma solidity 0.8.19;
 
 /// @title FixedPoint, a fixed-point arithmetic library defining the custom type uint192
 /// @author Matt Elder <[email protected]> and the Reserve Team <https://reserve.org>
@@ -11,7 +11,7 @@ pragma solidity ^0.8.19;
     "fixed192x18" -- a value represented by 192 bits, that makes 18 digits available to
     the right of the decimal point.
 
-    The range of values that uint192 can represent is about [-1.7e20, 1.7e20].
+    The range of values that uint192 can represent is [0, 2^192-1 / 10^18 = 6.2e39].
     Unless a function explicitly says otherwise, it will fail on overflow.
     To be clear, the following should hold:
     toFix(0) == 0
@@ -44,8 +44,8 @@ uint64 constant FIX_SCALE = 1e18;
 // FIX_SCALE Squared:
 uint128 constant FIX_SCALE_SQ = 1e36;
 
-// The largest integer that can be converted to uint192 .
-// This is a bit bigger than 3.1e39
+// The largest integer that can be converted to uint192.
+// This is a bit bigger than 6.2e39
 uint192 constant FIX_MAX_INT = type(uint192).max / FIX_SCALE;
 
 uint192 constant FIX_ZERO = 0; // The uint192 representation of zero.
@@ -100,7 +100,7 @@ function shiftl_toFix(
     // conditions for avoiding overflow
     if (x == 0) return 0;
     if (shiftLeft <= -96) return (rounding == CEIL ? 1 : 0); // 0 < uint.max / 10**77 < 0.5
-    if (40 <= shiftLeft) revert UIntOutOfBounds(); // 10**56 < FIX_MAX < 10**57
+    if (40 <= shiftLeft) revert UIntOutOfBounds(); // 10**57 < FIX_MAX < 10**58
 
     shiftLeft += 18;
 

contracts/p0/BackingManager.sol

@@ -3,6 +3,7 @@ pragma solidity 0.8.19;
 
 import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
 import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import "@openzeppelin/contracts/utils/math/Math.sol";
 import "./mixins/TradingLib.sol";
 import "./mixins/Trading.sol";
 import "../interfaces/IAsset.sol";
@@ -65,6 +66,8 @@ contract BackingManagerP0 is TradingP0, IBackingManager {
         trade = super.settleTrade(sell);
         delete tokensOut[trade.sell()];
 
+        tradeEnd[trade.KIND()] = uint48(Math.min(tradeEnd[trade.KIND()], block.timestamp));
+
         // if the settler is the trade contract itself, try chaining with another rebalance()
         if (_msgSender() == address(trade)) {
             // solhint-disable-next-line no-empty-blocks
@@ -97,7 +100,7 @@ contract BackingManagerP0 is TradingP0, IBackingManager {
         );
         require(!main.basketHandler().fullyCollateralized(), "already collateralized");
 
-        // First dissolve any held RToken balance above Distributor-dust
+        // First dissolve any held RToken balance
         // gas-optimization: 1 whole RToken must be worth 100 trillion dollars for this to skip $1
         uint256 balance = main.rToken().balanceOf(address(this));
         if (balance >= MAX_DISTRIBUTION * MAX_DESTINATIONS) main.rToken().dissolve(balance);
@@ -246,7 +249,16 @@ contract BackingManagerP0 is TradingP0, IBackingManager {
         assert(main.basketHandler().fullyCollateralized());
     }
 
-    // === Setters ===
+    // === Governance ===
+
+    /// Forcibly settle a trade, losing all value
+    /// Should only be called in case of censorship
+    /// @param trade The trade address itself
+    /// @custom:governance
+    function forceSettleTrade(ITrade trade) public override(TradingP0, ITrading) governance {
+        super.forceSettleTrade(trade);
+        delete tokensOut[trade.sell()];
+    }
 
     /// @custom:governance
     function setTradingDelay(uint48 val) public governance {

contracts/p0/BasketHandler.sol

@@ -113,6 +113,7 @@ contract BasketHandlerP0 is ComponentP0, IBasketHandler {
 
     uint48 public constant MIN_WARMUP_PERIOD = 60; // {s} 1 minute
     uint48 public constant MAX_WARMUP_PERIOD = 31536000; // {s} 1 year
+    uint192 public constant MIN_TARGET_AMT = FIX_ONE / 1e6; // {target/BU} min basket weight: 1e-6
     uint192 public constant MAX_TARGET_AMT = 1e3 * FIX_ONE; // {target/BU} max basket weight
     uint256 internal constant MAX_BACKUP_ERC20S = 64;
 
@@ -268,7 +269,8 @@ contract BasketHandlerP0 is ComponentP0, IBasketHandler {
     /// Set the prime basket in the basket configuration, in terms of erc20s and target amounts
     /// @param erc20s The collateral for the new prime basket
     /// @param targetAmts The target amounts (in) {target/BU} for the new prime basket
-    /// @param disableTargetAmountCheck If true, skips the `requireConstantConfigTargets()` check
+    /// @param disableTargetAmountCheck For reweightable RTokens, if true
+    ///                                 skips the `requireConstantConfigTargets()` check
     /// @custom:governance
     // checks:
     //   caller is OWNER
@@ -313,7 +315,7 @@ contract BasketHandlerP0 is ComponentP0, IBasketHandler {
             // This is a nice catch to have, but in general it is possible for
             // an ERC20 in the prime basket to have its asset unregistered.
             require(reg.toAsset(erc20s[i]).isCollateral(), "erc20 is not collateral");
-            require(0 < targetAmts[i], "invalid target amount");
+            require(MIN_TARGET_AMT <= targetAmts[i], "invalid target amount");
             require(targetAmts[i] <= MAX_TARGET_AMT, "invalid target amount");
 
             config.erc20s.push(erc20s[i]);

contracts/p0/Broker.sol

@@ -30,8 +30,6 @@ contract BrokerP0 is ComponentP0, IBroker {
     ITrade public batchTradeImplementation;
     ITrade public dutchTradeImplementation;
 
-    IGnosis public gnosis;
-
     mapping(address => bool) private trades;
 
     uint48 public batchAuctionLength; // {s} the length of a Gnosis EasyAuction
@@ -43,14 +41,12 @@ contract BrokerP0 is ComponentP0, IBroker {
 
     function init(
         IMain main_,
-        IGnosis gnosis_,
         ITrade batchTradeImplementation_, // Added for Interface compatibility with P1
         uint48 batchAuctionLength_,
         ITrade dutchTradeImplementation_, // Added for Interface compatibility with P1
         uint48 dutchAuctionLength_
     ) public initializer {
         __Component_init(main_);
-        setGnosis(gnosis_);
         setBatchTradeImplementation(batchTradeImplementation_);
         setBatchAuctionLength(batchAuctionLength_);
         setDutchTradeImplementation(dutchTradeImplementation_);
@@ -133,14 +129,6 @@ contract BrokerP0 is ComponentP0, IBroker {
 
     // === Setters ===
 
-    /// @custom:governance
-    function setGnosis(IGnosis newGnosis) public governance {
-        require(address(newGnosis) != address(0), "invalid Gnosis address");
-
-        emit GnosisSet(gnosis, newGnosis);
-        gnosis = newGnosis;
-    }
-
     /// @custom:governance
     function setBatchTradeImplementation(ITrade newTradeImplementation) public governance {
         require(
@@ -220,7 +208,7 @@ contract BrokerP0 is ComponentP0, IBroker {
             req.sellAmount
         );
 
-        trade.init(this, caller, gnosis, batchAuctionLength, req);
+        trade.init(this, caller, batchAuctionLength, req);
         return trade;
     }
 

contracts/p0/Deployer.sol

@@ -119,8 +119,7 @@ contract DeployerP0 is IDeployer, Versioned {
 
         main.broker().init(
             main,
-            gnosis,
-            ITrade(address(new GnosisTrade())),
+            ITrade(address(new GnosisTrade(gnosis))),
             params.batchAuctionLength,
             ITrade(address(new DutchTrade())),
             params.dutchAuctionLength

contracts/p0/Distributor.sol

@@ -139,7 +139,11 @@ contract DistributorP0 is ComponentP0, IDistributor {
         require(dest != address(0), "dest cannot be zero");
         require(
             dest != address(main.furnace()) && dest != address(main.stRSR()),
-            "destination can not be furnace or strsr directly"
+            "destination cannot be furnace or strsr directly"
+        );
+        require(
+            dest != address(main.rsr()) && dest != address(main.rToken()),
+            "destination cannot be rsr or rToken"
         );
         require(dest != address(main.daoFeeRegistry()), "destination cannot be daoFeeRegistry");
         if (dest == FURNACE) require(share.rsrDist == 0, "Furnace must get 0% of RSR");

contracts/p0/RToken.sol

@@ -140,6 +140,7 @@ contract RTokenP0 is ComponentP0, ERC20PermitUpgradeable, IRToken {
         main.poke();
 
         require(amount > 0, "Cannot redeem zero");
+        require(recipient != address(0), "cannot redeem to zero address");
         require(amount <= balanceOf(_msgSender()), "insufficient balance");
         require(main.basketHandler().fullyCollateralized(), "partial redemption; use redeemCustom");
         // redemption while IFFY/DISABLED allowed
@@ -267,6 +268,8 @@ contract RTokenP0 is ComponentP0, ERC20PermitUpgradeable, IRToken {
     /// @custom:protected
     function mint(uint192 baskets) external exchangeRateIsValidAfter {
         require(_msgSender() == address(main.backingManager()), "not backing manager");
+        issuanceThrottle.useAvailable(totalSupply(), 0);
+        redemptionThrottle.useAvailable(totalSupply(), 0);
         _scaleUp(address(main.backingManager()), baskets);
     }
 
@@ -285,6 +288,8 @@ contract RTokenP0 is ComponentP0, ERC20PermitUpgradeable, IRToken {
     /// @custom:protected
     function dissolve(uint256 amount) external exchangeRateIsValidAfter {
         require(_msgSender() == address(main.backingManager()), "not backing manager");
+        issuanceThrottle.useAvailable(totalSupply(), 0);
+        redemptionThrottle.useAvailable(totalSupply(), 0);
         _scaleDown(_msgSender(), amount);
     }
 

contracts/p0/StRSR.sol

@@ -102,6 +102,8 @@ contract StRSRP0 is IStRSR, ComponentP0, EIP712Upgradeable {
     // stake rate under/over which governance can reset all stakes
     uint192 private constant MAX_SAFE_STAKE_RATE = 1e6 * FIX_ONE; // 1e6
     uint192 private constant MIN_SAFE_STAKE_RATE = uint192(1e12); // 1e-6
+    uint192 private constant MAX_SAFE_DRAFT_RATE = 1e6 * FIX_ONE; // 1e6
+    uint192 private constant MIN_SAFE_DRAFT_RATE = uint192(1e12); // 1e-6
 
     // Withdrawal Leak
     uint192 private leaked; // {1} stake fraction that has withdrawn without a refresh
@@ -272,8 +274,7 @@ contract StRSRP0 is IStRSR, ComponentP0, EIP712Upgradeable {
         uint256 i = start;
         for (; i < endId; i++) {
             total += queue[i].rsrAmount;
-            queue[i].rsrAmount = 0;
-            queue[i].stakeAmount = 0;
+            delete queue[i];
         }
 
         // Execute accumulated withdrawals
@@ -387,10 +388,15 @@ contract StRSRP0 is IStRSR, ComponentP0, EIP712Upgradeable {
     /// @custom:governance
     /// Reset all stakes and advance era
     function resetStakes() external governance {
+        uint256 rsrDrafts = rsrBeingWithdrawn();
+        uint192 draftRate = rsrDrafts > 0 ? divuu(stakeBeingWithdrawn(), rsrDrafts) : FIX_ONE;
         uint192 stakeRate = divuu(totalStaked, rsrBacking);
         require(
-            stakeRate <= MIN_SAFE_STAKE_RATE || stakeRate >= MAX_SAFE_STAKE_RATE,
-            "rate still safe"
+            draftRate <= MIN_SAFE_DRAFT_RATE ||
+                draftRate >= MAX_SAFE_DRAFT_RATE ||
+                stakeRate <= MIN_SAFE_STAKE_RATE ||
+                stakeRate >= MAX_SAFE_STAKE_RATE,
+            "rates still safe"
         );
 
         bankruptStakers();

contracts/p0/mixins/Trading.sol

@@ -94,11 +94,24 @@ abstract contract TradingP0 is RewardableP0, ITrading {
         );
     }
 
-    // === Setters ===
+    // === Governance ===
+
+    /// Forcibly settle a trade, losing all value
+    /// Should only be called in case of censorship
+    /// @param trade The trade address itself
+    /// @custom:governance
+    function forceSettleTrade(ITrade trade) public virtual governance {
+        // should not call any ERC20 functions, in case bricked
+
+        IERC20Metadata sell = trade.sell();
+        delete trades[sell];
+        tradesOpen--;
+        emit TradeSettled(trade, sell, trade.buy(), 0, 0);
+    }
 
     /// @custom:governance
     function setMaxTradeSlippage(uint192 val) public governance {
-        require(val < MAX_TRADE_SLIPPAGE, "invalid maxTradeSlippage");
+        require(val <= MAX_TRADE_SLIPPAGE, "invalid maxTradeSlippage");
         emit MaxTradeSlippageSet(maxTradeSlippage, val);
         maxTradeSlippage = val;
     }

contracts/p0/mixins/TradingLib.sol

@@ -60,7 +60,7 @@ library TradingLibP0 {
         );
 
         // Cap sell amount using the high price
-        // Under price decay trade.prices.sellHigh can become up to 3x the savedHighPrice before
+        // Under price decay trade.prices.sellHigh can become up to 2x the savedHighPrice before
         // becoming FIX_MAX after the full price timeout
         uint192 s = trade.sellAmount;
         if (trade.prices.sellHigh != FIX_MAX) {