QA (#1188)

common/configuration.ts

@@ -706,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/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

@@ -97,7 +97,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);

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/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

contracts/p0/mixins/Trading.sol

@@ -111,7 +111,7 @@ abstract contract TradingP0 is RewardableP0, ITrading {
 
     /// @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) {

contracts/p1/BackingManager.sol

@@ -125,7 +125,7 @@ contract BackingManagerP1 is TradingP1, IBackingManager {
         require(basketsHeld.bottom < rToken.basketsNeeded(), "already collateralized");
         // require(!basketHandler.fullyCollateralized())
 
-        // 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 = rToken.balanceOf(address(this));
         if (balance >= MAX_DISTRIBUTION * MAX_DESTINATIONS) rToken.dissolve(balance);

contracts/p1/BasketHandler.sol

@@ -28,7 +28,8 @@ contract BasketHandlerP1 is ComponentP1, IBasketHandler {
     using EnumerableSet for EnumerableSet.Bytes32Set;
     using FixLib for uint192;
 
-    uint192 public constant MAX_TARGET_AMT = 1e3 * FIX_ONE; // {target/BU} max basket weight
+    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: 1e3
     uint48 public constant MIN_WARMUP_PERIOD = 60; // {s} 1 minute
     uint48 public constant MAX_WARMUP_PERIOD = 60 * 60 * 24 * 365; // {s} 1 year
     uint256 internal constant MAX_BACKUP_ERC20S = 64;
@@ -211,7 +212,8 @@ contract BasketHandlerP1 is ComponentP1, 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
@@ -261,7 +263,10 @@ contract BasketHandlerP1 is ComponentP1, 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(assetRegistry.toAsset(erc20s[i]).isCollateral(), "erc20 is not collateral");
-            require(0 < targetAmts[i] && targetAmts[i] <= MAX_TARGET_AMT, "invalid target amount");
+            require(
+                MIN_TARGET_AMT <= targetAmts[i] && targetAmts[i] <= MAX_TARGET_AMT,
+                "invalid target amount"
+            );
 
             config.erc20s.push(erc20s[i]);
             config.targetAmts[erc20s[i]] = targetAmts[i];

contracts/p1/Furnace.sol

@@ -58,7 +58,7 @@ contract FurnaceP1 is ComponentP1, IFurnace {
     //     payoutAmount = RToken.balanceOf(this) * (1 - (1-ratio)**N) from [furnace-payout-formula]
     // effects:
     //   lastPayout' = lastPayout + numPeriods (end of last pay period)
-    //   lastPayoutBal' = rToken.balanceOf'(this) (balance now == at end of pay leriod)
+    //   lastPayoutBal' = rToken.balanceOf'(this) (balance now == at end of pay period)
     // actions:
     //   rToken.melt(payoutAmount), paying payoutAmount to RToken holders
 

contracts/p1/RToken.sol

@@ -189,6 +189,7 @@ contract RTokenP1 is ComponentP1, ERC20PermitUpgradeable, IRToken {
         address caller = _msgSender();
 
         require(amount != 0, "Cannot redeem zero");
+        require(recipient != address(0), "cannot redeem to zero address");
         require(amount <= balanceOf(caller), "insufficient balance");
         require(basketHandler.fullyCollateralized(), "partial redemption; use redeemCustom");
         // redemption while IFFY/DISABLED allowed

contracts/p1/StRSR.sol

@@ -326,10 +326,10 @@ abstract contract StRSRP1 is Initializable, ComponentP1, IStRSR, EIP712Upgradeab
         uint256 newDraftRSR = (newTotalDrafts * FIX_ONE_256 + (draftRate - 1)) / draftRate;
         uint256 rsrAmount = draftRSR - newDraftRSR;
 
-        if (rsrAmount == 0) return;
-
         // ==== Transfer RSR from the draft pool
         totalDrafts = newTotalDrafts;
+        if (rsrAmount == 0) return;
+
         draftRSR = newDraftRSR;
 
         // == Interactions ==

contracts/p1/mixins/TradeLib.sol

@@ -59,7 +59,7 @@ library TradeLib {
         );
 
         // 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) {

contracts/p1/mixins/Trading.sol

@@ -157,7 +157,7 @@ abstract contract TradingP1 is Multicall, ComponentP1, ReentrancyGuardUpgradeabl
 
     /// @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/plugins/assets/Asset.sol

@@ -11,7 +11,7 @@ contract Asset is IAsset, VersionedAsset {
     using FixLib for uint192;
     using OracleLib for AggregatorV3Interface;
 
-    uint192 public constant MAX_HIGH_PRICE_BUFFER = 2 * FIX_ONE; // {UoA/tok} 200%
+    uint192 public constant MAX_HIGH_PRICE_BUFFER = FIX_ONE; // {UoA/tok} 100%
 
     AggregatorV3Interface public immutable chainlinkFeed; // {UoA/tok}
 
@@ -147,7 +147,7 @@ contract Asset is IAsset, VersionedAsset {
             } else {
                 // decayDelay <= delta <= decayDelay + priceTimeout
 
-                // Decay _high upwards to 3x savedHighPrice
+                // Decay _high upwards to 2x savedHighPrice
                 // {UoA/tok} = {UoA/tok} * {1}
                 _high = savedHighPrice.safeMul(
                     FIX_ONE + MAX_HIGH_PRICE_BUFFER.muluDivu(delta - decayDelay, priceTimeout),

contracts/plugins/trading/DutchTrade.sol

@@ -165,8 +165,14 @@ contract DutchTrade is ITrade, Versioned {
         assert(address(sell_) != address(0) && address(buy_) != address(0) && auctionLength >= 60);
 
         // Only start dutch auctions under well-defined prices
-        require(prices.sellLow != 0 && prices.sellHigh < FIX_MAX / 1000, "bad sell pricing");
-        require(prices.buyLow != 0 && prices.buyHigh < FIX_MAX / 1000, "bad buy pricing");
+        require(
+            prices.sellLow != 0 && prices.sellHigh != 0 && prices.sellHigh < FIX_MAX / 1000,
+            "bad sell pricing"
+        );
+        require(
+            prices.buyLow != 0 && prices.buyHigh != 0 && prices.buyHigh < FIX_MAX / 1000,
+            "bad buy pricing"
+        );
 
         broker = IBroker(msg.sender);
         origin = origin_;

contracts/plugins/trading/GnosisTrade.sol

@@ -47,7 +47,7 @@ contract GnosisTrade is ITrade, Versioned {
 
     // == Economic parameters
     // This trade is on behalf of origin. Only origin may call settle(), and the `buy` tokens
-    // from this trade's acution will all eventually go to origin.
+    // from this trade's auction will all eventually go to origin.
     address public origin;
     IERC20Metadata public sell; // address of token this trade is selling
     IERC20Metadata public buy; // address of token this trade is buying
@@ -56,7 +56,7 @@ contract GnosisTrade is ITrade, Versioned {
     uint48 public endTime; // timestamp after which this trade's auction can be settled
     uint192 public worstCasePrice; // D27{qBuyTok/qSellTok}, the worst price we expect to get
     // We expect Gnosis Auction either to meet or beat worstCasePrice, or to return the `sell`
-    // tokens. If we actually *get* a worse clearing that worstCasePrice, we consider it an error in
+    // tokens. If we actually *get* a worse clearing than worstCasePrice, we consider it an error in
     // our trading scheme and call broker.reportViolation()
 
     // This modifier both enforces the state-machine pattern and guards against reentrancy.

docs/recollateralization.md

@@ -60,7 +60,7 @@ This allows the protocol to deterministically select the next trade based on the
    (Caveat: if the protocol gets an unreasonably good trade in excess of what was indicated by an asset's price range, this can happen)
 5. Large trades first, as determined by comparison in the `{UoA}`
 
-If there does not exist a trade that meets these constraints, then the protocol "takes a haircut", which is a colloquial way of saying it reduces `RToken.basketsNeeded()` to its current BU holdings. This causes a loss for RToken holders (undesirable) but causes the protocol to become collateralized again, allowing it to re-enter into a period of normal operation.
+If there does not exist a trade that meets these constraints, the protocol considers the RSR balance in StRSR before moving to "take a haircut", which is a colloquial way of saying it reduces `RToken.basketsNeeded()` to its current BU holdings to become by-definition collateralized. This causes a loss for RToken holders (undesirable) but causes the protocol to regain normal function.
 
 #### Trade Sizing
 

test/Main.test.ts

@@ -8,6 +8,7 @@ import {
   MAX_TRADING_DELAY,
   MAX_TRADE_SLIPPAGE,
   MAX_BACKING_BUFFER,
+  MIN_TARGET_AMT,
   MAX_TARGET_AMT,
   MAX_MIN_TRADE_VOLUME,
   MIN_WARMUP_PERIOD,
@@ -1039,7 +1040,7 @@ describe(`MainP${IMPLEMENTATION} contract`, () => {
 
       // Cannot update with value > max
       await expect(
-        backingManager.connect(owner).setMaxTradeSlippage(MAX_TRADE_SLIPPAGE)
+        backingManager.connect(owner).setMaxTradeSlippage(MAX_TRADE_SLIPPAGE.add(1))
       ).to.be.revertedWith('invalid maxTradeSlippage')
     })
 
@@ -2157,6 +2158,16 @@ describe(`MainP${IMPLEMENTATION} contract`, () => {
         ).to.be.revertedWith('invalid collateral')
       })
 
+      it('Should not allow to bypass MIN_TARGET_AMT', async () => {
+        // not possible on non-fresh basketHandler
+        await expect(
+          indexBH.connect(owner).setPrimeBasket([token0.address], [MIN_TARGET_AMT.sub(1)])
+        ).to.be.revertedWith('invalid target amount')
+        await expect(
+          indexBH.connect(owner).forceSetPrimeBasket([token0.address], [MIN_TARGET_AMT.sub(1)])
+        ).to.be.revertedWith('invalid target amount')
+      })
+
       it('Should not allow to bypass MAX_TARGET_AMT', async () => {
         // not possible on non-fresh basketHandler
         await expect(

test/RToken.test.ts

@@ -319,6 +319,13 @@ describe(`RTokenP${IMPLEMENTATION} contract`, () => {
       expect(endPrice[0]).to.eq(0)
       expect(endPrice[1]).to.eq(0)
     })
+
+    it('Should not allow redemption to zero address', async function () {
+      // Redeem rTokens to zero address
+      await expect(rToken.connect(addr1).redeemTo(ZERO_ADDRESS, fp('1'))).to.be.revertedWith(
+        'cannot redeem to zero address'
+      )
+    })
   })
 
   describe('Issuance', function () {

test/ZTradingExtremes.test.ts

@@ -5,6 +5,7 @@ import { BigNumber, ContractFactory } from 'ethers'
 import { ethers } from 'hardhat'
 import {
   IConfig,
+  MIN_TARGET_AMT,
   MAX_ORACLE_TIMEOUT,
   MAX_THROTTLE_AMT_RATE,
   MAX_BASKET_SIZE,
@@ -415,7 +416,11 @@ describeExtreme(`Trading Extreme Values (${SLOW ? 'slow mode' : 'fast mode'})`,
         }
 
         primeBasket.push(token)
-        targetAmts.push(divCeil(primeWeight, bn(basketSize))) // might sum to slightly over, is ok
+
+        let targetAmt = divCeil(primeWeight, bn(basketSize))
+        if (targetAmt.lt(MIN_TARGET_AMT)) targetAmt = MIN_TARGET_AMT
+        targetAmts.push(targetAmt) // might sum to slightly over, is ok
+
         await token.connect(owner).mint(addr1.address, MAX_UINT256)
         await token.connect(addr1).approve(rToken.address, MAX_UINT256)
       }
@@ -788,7 +793,11 @@ describeExtreme(`Trading Extreme Values (${SLOW ? 'slow mode' : 'fast mode'})`,
         }
 
         primeBasket.push(token)
-        targetAmts.push(primeWeight.div(basketSize).add(1))
+
+        let targetAmt = divCeil(primeWeight, bn(basketSize))
+        if (targetAmt.lt(MIN_TARGET_AMT)) targetAmt = MIN_TARGET_AMT
+        targetAmts.push(targetAmt) // might sum to slightly over, is ok
+
         await token.connect(owner).mint(addr1.address, MAX_UINT256)
         await token.connect(addr1).approve(rToken.address, MAX_UINT256)
       }
@@ -938,7 +947,7 @@ describeExtreme(`Trading Extreme Values (${SLOW ? 'slow mode' : 'fast mode'})`,
         const erc20 = await makeToken(`Token ${i}`, targetUnit, targetPerRefs)
         primeERC20s.push(erc20.address)
         let targetAmt = basketTargetAmt.div(targetUnits)
-        if (targetAmt.eq(bn(0))) targetAmt = bn(1)
+        if (targetAmt.lt(MIN_TARGET_AMT)) targetAmt = MIN_TARGET_AMT
         targetAmts.push(targetAmt)
       }
 

test/plugins/individual-collateral/collateralTests.ts

@@ -428,8 +428,8 @@ export default function fn<X extends CollateralFixtureContext>(
           const priceTimeout = await collateral.priceTimeout()
           await advanceTime(priceTimeout / 2)
           p = await collateral.price()
-          expect(p[0]).to.be.closeTo(savedLow.div(2), p[0].div(2).div(10000)) // 1 part in 10 thousand
-          expect(p[1]).to.be.closeTo(savedHigh.mul(2), p[1].mul(2).div(10000)) // 1 part in 10 thousand
+          expect(p[0]).to.be.closeTo(savedLow.div(2), savedLow.div(2).div(10000)) // 1 part in 10 thousand
+          expect(p[1]).to.be.closeTo(savedHigh.mul(3).div(2), savedHigh.mul(3).div(2).div(10000)) // 1 part in 10k
 
           // Should be unpriced after full priceTimeout
           await advanceTime(priceTimeout / 2)

test/plugins/individual-collateral/curve/collateralTests.ts

@@ -500,8 +500,8 @@ export default function fn<X extends CurveCollateralFixtureContext>(
           const priceTimeout = await ctx.collateral.priceTimeout()
           await advanceTime(priceTimeout / 2)
           p = await ctx.collateral.price()
-          expect(p[0]).to.be.closeTo(savedLow.div(2), p[0].div(2).div(10000)) // 1 part in 10 thousand
-          expect(p[1]).to.be.closeTo(savedHigh.mul(2), p[1].mul(2).div(10000)) // 1 part in 10 thousand
+          expect(p[0]).to.be.closeTo(savedLow.div(2), savedLow.div(2).div(10000)) // 1 part in 10 thousand
+          expect(p[1]).to.be.closeTo(savedHigh.mul(3).div(2), savedHigh.mul(3).div(2).div(10000)) // 1 part in 10k
 
           // Should be 0 after full priceTimeout
           await advanceTime(priceTimeout / 2)