Coverage 3.3.0 - Phase 1 (#1083)

contracts/libraries/Allowance.sol

@@ -23,6 +23,8 @@ library AllowanceLib {
 
         // 1. Set initial allowance to 0
         token.approve(spender, 0);
+        // untestable:
+        //    allowance should always be 0 if token behaves correctly
         require(token.allowance(address(this), spender) == 0, "allowance not 0");
 
         if (value == 0) return;
@@ -37,6 +39,8 @@ library AllowanceLib {
         // 3. Fall-back to setting a maximum allowance
         if (!success) {
             token.approve(spender, type(uint256).max);
+            // untestable:
+            //    allowance should always be max value if token behaves correctly
             require(token.allowance(address(this), spender) >= value, "allowance missing");
         }
     }

contracts/p1/Broker.sol

@@ -158,6 +158,7 @@ contract BrokerP1 is ComponentP1, IBroker {
                 dutchTradeDisabled[buy] = true;
             }
         } else {
+            // untestable: trade kind is either BATCH or DUTCH
             revert("unrecognized trade kind");
         }
     }

contracts/p1/Distributor.sol

@@ -124,10 +124,10 @@ contract DistributorP1 is ComponentP1, IDistributor {
 
             if (addrTo == FURNACE) {
                 addrTo = furnaceAddr;
-                if (transferAmt > 0) accountRewards = true;
+                accountRewards = true;
             } else if (addrTo == ST_RSR) {
                 addrTo = stRSRAddr;
-                if (transferAmt > 0) accountRewards = true;
+                accountRewards = true;
             }
 
             transfers[numTransfers] = Transfer({ addrTo: addrTo, amount: transferAmt });

contracts/p1/RevenueTrader.sol

@@ -56,6 +56,8 @@ contract RevenueTraderP1 is TradingP1, IRevenueTrader {
 
         // solhint-disable-next-line no-empty-blocks
         try this.distributeTokenToBuy() {} catch (bytes memory errData) {
+            // untested:
+            //     OOG pattern tested in other contracts, cost to test here is high
             // see: docs/solidity-style.md#Catching-Empty-Data
             if (errData.length == 0) revert(); // solhint-disable-line reason-string
         }

contracts/p1/mixins/BasketLib.sol

@@ -363,6 +363,8 @@ library BasketLibP1 {
             ICollateral coll = assetRegistry.toColl(erc20s[i]); // reverts if unregistered
 
             (uint192 low, uint192 high) = coll.price(); // {UoA/tok}
+            // untestable:
+            //      this function is only called if basket is SOUND
             require(low > 0 && high < FIX_MAX, "invalid price");
 
             // {UoA/BU} += {target/BU} * {UoA/tok} / ({target/ref} * {ref/tok})

contracts/plugins/assets/EURFiatCollateral.sol

@@ -55,6 +55,8 @@ contract EURFiatCollateral is FiatCollateral {
         uint192 pricePerTarget = targetUnitChainlinkFeed.price(targetUnitOracleTimeout);
 
         // div-by-zero later
+        // untestable:
+        //      calls to price() on the feed never return zero if using OracleLib
         if (pricePerTarget == 0) {
             return (0, FIX_MAX, 0);
         }

contracts/plugins/assets/RTokenAsset.sol

@@ -150,6 +150,8 @@ contract RTokenAsset is IAsset, VersionedAsset, IRTokenOracle {
     /// @return updatedAt {s} The timestamp of the cache update
     function latestPrice() external returns (uint192 rTokenPrice, uint256 updatedAt) {
         // Situations that require an update, from most common to least common.
+        // untestable:
+        //     basket and trade nonce checks, as first condition will always be true in these cases
         if (
             cachedOracleData.cachedAtTime + ORACLE_TIMEOUT <= block.timestamp || // Cache Timeout
             cachedOracleData.cachedAtNonce != basketHandler.nonce() || // Basket nonce was updated

contracts/plugins/assets/compoundv2/CTokenNonFiatCollateral.sol

@@ -31,7 +31,6 @@ contract CTokenNonFiatCollateral is CTokenFiatCollateral {
     ) CTokenFiatCollateral(config, revenueHiding) {
         require(address(targetUnitChainlinkFeed_) != address(0), "missing targetUnit feed");
         require(targetUnitOracleTimeout_ > 0, "targetUnitOracleTimeout zero");
-        require(config.defaultThreshold > 0, "defaultThreshold zero");
         targetUnitChainlinkFeed = targetUnitChainlinkFeed_;
         targetUnitOracleTimeout = targetUnitOracleTimeout_;
         maxOracleTimeout = uint48(Math.max(maxOracleTimeout, targetUnitOracleTimeout_));

contracts/plugins/mocks/CTokenMock.sol

@@ -12,7 +12,8 @@ contract CTokenMock is ERC20Mock {
 
     uint256 internal _exchangeRate;
 
-    bool public revertExchangeRate;
+    bool public revertExchangeRateCurrent;
+    bool public revertExchangeRateStored;
 
     IComptroller public immutable comptroller;
 
@@ -32,14 +33,17 @@ contract CTokenMock is ERC20Mock {
     }
 
     function exchangeRateCurrent() external returns (uint256) {
-        if (revertExchangeRate) {
+        if (revertExchangeRateCurrent) {
             revert("reverting exchange rate current");
         }
         _exchangeRate = _exchangeRate; // just to avoid sol warning
         return _exchangeRate;
     }
 
     function exchangeRateStored() external view returns (uint256) {
+         if (revertExchangeRateStored) {
+            revert("reverting exchange rate stored");
+        }
         return _exchangeRate;
     }
 
@@ -59,7 +63,11 @@ contract CTokenMock is ERC20Mock {
         return fiatcoinRedemptionRate.shiftl(leftShift).mul_toUint(start);
     }
 
-    function setRevertExchangeRate(bool newVal) external {
-        revertExchangeRate = newVal;
+    function setRevertExchangeRateCurrent(bool newVal) external {
+        revertExchangeRateCurrent = newVal;
+    }
+
+    function setRevertExchangeRateStored(bool newVal) external {
+        revertExchangeRateStored = newVal;
     }
 }

contracts/plugins/mocks/InvalidChainlinkMock.sol

@@ -11,10 +11,12 @@ import "./ChainlinkMock.sol";
  */
 contract InvalidMockV3Aggregator is MockV3Aggregator {
     bool public simplyRevert;
+    bool public revertWithExplicitError;
 
-    constructor(uint8 _decimals, int256 _initialAnswer)
-        MockV3Aggregator(_decimals, _initialAnswer)
-    {}
+    constructor(
+        uint8 _decimals,
+        int256 _initialAnswer
+    ) MockV3Aggregator(_decimals, _initialAnswer) {}
 
     function latestRoundData()
         external
@@ -30,6 +32,8 @@ contract InvalidMockV3Aggregator is MockV3Aggregator {
     {
         if (simplyRevert) {
             revert(); // Revert with no reason
+        } else if (revertWithExplicitError) {
+            revert("oracle explicit error"); // Revert with explicit reason
         } else {
             // Run out of gas
             this.infiniteLoop{ gas: 10 }();
@@ -47,6 +51,10 @@ contract InvalidMockV3Aggregator is MockV3Aggregator {
         simplyRevert = on;
     }
 
+    function setRevertWithExplicitError(bool on) external {
+        revertWithExplicitError = on;
+    }
+
     function infiniteLoop() external pure {
         uint256 i = 0;
         uint256[1] memory array;

contracts/plugins/mocks/UnpricedPlugins.sol

@@ -27,23 +27,51 @@ contract UnpricedAssetMock is Asset {
         uint48 oracleTimeout_
     ) Asset(priceTimeout_, chainlinkFeed_, oracleError_, erc20_, maxTradeVolume_, oracleTimeout_) {}
 
+    /// tryPrice: mock unpriced by returning (0, FIX_MAX)
+    function tryPrice() external view override returns (uint192 low, uint192 high, uint192) {
+        // If unpriced is marked, return 0, FIX_MAX
+        if (unpriced) return (0, FIX_MAX, 0);
+
+        uint192 p = chainlinkFeed.price(oracleTimeout); // {UoA/tok}
+        uint192 delta = p.mul(oracleError, CEIL);
+        return (p - delta, p + delta, 0);
+    }
+
+    function setUnpriced(bool on) external {
+        unpriced = on;
+    }
+}
+
+contract UnpricedFiatCollateralMock is FiatCollateral {
+    using FixLib for uint192;
+    using OracleLib for AggregatorV3Interface;
+
+    bool public unpriced = false;
+
+    // solhint-disable no-empty-blocks
+
+    constructor(CollateralConfig memory config) FiatCollateral(config) {}
+
     /// tryPrice: mock unpriced by returning (0, FIX_MAX)
     function tryPrice()
         external
         view
+        virtual
         override
-        returns (
-            uint192 low,
-            uint192 high,
-            uint192
-        )
+        returns (uint192 low, uint192 high, uint192 pegPrice)
     {
         // If unpriced is marked, return 0, FIX_MAX
         if (unpriced) return (0, FIX_MAX, 0);
 
-        uint192 p = chainlinkFeed.price(oracleTimeout); // {UoA/tok}
-        uint192 delta = p.mul(oracleError, CEIL);
-        return (p - delta, p + delta, 0);
+        // {target/ref} = {UoA/ref} / {UoA/target} (1)
+        pegPrice = chainlinkFeed.price(oracleTimeout);
+
+        // {target/ref} = {target/ref} * {1}
+        uint192 err = pegPrice.mul(oracleError, CEIL);
+
+        low = pegPrice - err;
+        high = pegPrice + err;
+        // assert(low <= high); obviously true just by inspection
     }
 
     function setUnpriced(bool on) external {
@@ -62,21 +90,18 @@ contract UnpricedAppreciatingFiatCollateralMock is AppreciatingFiatCollateral {
     // solhint-disable no-empty-blocks
 
     /// @param config.chainlinkFeed Feed units: {UoA/ref}
-    constructor(CollateralConfig memory config, uint192 revenueHiding)
-        AppreciatingFiatCollateral(config, revenueHiding)
-    {}
+    constructor(
+        CollateralConfig memory config,
+        uint192 revenueHiding
+    ) AppreciatingFiatCollateral(config, revenueHiding) {}
 
     /// tryPrice: mock unpriced by returning (0, FIX_MAX)
     function tryPrice()
         external
         view
         virtual
         override
-        returns (
-            uint192 low,
-            uint192 high,
-            uint192 pegPrice
-        )
+        returns (uint192 low, uint192 high, uint192 pegPrice)
     {
         // If unpriced is marked, return 0, FIX_MAX
         if (unpriced) return (0, FIX_MAX, 0);

test/Facade.test.ts

@@ -446,6 +446,27 @@ describe('Facade + FacadeMonitor contracts', () => {
       await expect(facade.callStatic.redeem(rToken.address, issueAmount)).to.be.revertedWith(
         'frozen'
       )
+
+      await expect(
+        facade.callStatic.redeemCustom(
+          rToken.address,
+          issueAmount,
+          [await basketHandler.nonce()],
+          [fp('1')]
+        )
+      ).to.be.revertedWith('frozen')
+    })
+
+    it('Should revert if portions do not sum to FIX_ONE in redeem custom', async function () {
+      const nonce = await basketHandler.nonce()
+      await expect(
+        facade.callStatic.redeemCustom(
+          rToken.address,
+          issueAmount,
+          [nonce, nonce],
+          [fp('0.5'), fp('0.5').add(1)]
+        )
+      ).to.be.revertedWith('portions do not add up to FIX_ONE')
     })
 
     it('Should return backingOverview correctly', async () => {

test/Main.test.ts

@@ -2195,6 +2195,22 @@ describe(`MainP${IMPLEMENTATION} contract`, () => {
       expect(tokAmts[1]).to.equal(fp('0.5'))
     })
 
+    it('Should handle unpriced asset in normalization', async () => {
+      await indexBH.connect(owner).setPrimeBasket([token0.address], [fp('1')])
+      await indexBH.connect(owner).refreshBasket()
+
+      // Set Token0 to unpriced - stale oracle
+      await advanceTime(DECAY_DELAY.add(PRICE_TIMEOUT).toString())
+      await expectUnpriced(collateral0.address)
+
+      // Attempt to add EURO, basket is not SOUND
+      await expect(
+        indexBH
+          .connect(owner)
+          .setPrimeBasket([token0.address, eurToken.address], [fp('1'), fp('0.25')])
+      ).to.be.revertedWith('unsound basket')
+    })
+
     describe('Custom Redemption', () => {
       const issueAmount = fp('10000')
       let usdcChainlink: MockV3Aggregator

test/Revenues.test.ts

@@ -3283,6 +3283,40 @@ describe(`Revenues - P${IMPLEMENTATION}`, () => {
         )
       })
 
+      it('Should support custom destinations only', async () => {
+        // Set distribution all to a custom destination
+        await expect(
+          distributor
+            .connect(owner)
+            .setDistribution(other.address, { rTokenDist: bn(0), rsrDist: bn(1) })
+        )
+          .to.emit(distributor, 'DistributionSet')
+          .withArgs(other.address, bn(0), bn(1))
+
+        // No distribution to Furnace or StRSR
+        await expect(
+          distributor
+            .connect(owner)
+            .setDistribution(FURNACE_DEST, { rTokenDist: bn(0), rsrDist: bn(0) })
+        )
+          .to.emit(distributor, 'DistributionSet')
+          .withArgs(FURNACE_DEST, bn(0), bn(0))
+
+        await expect(
+          distributor
+            .connect(owner)
+            .setDistribution(STRSR_DEST, { rTokenDist: bn(0), rsrDist: bn(0) })
+        )
+          .to.emit(distributor, 'DistributionSet')
+          .withArgs(STRSR_DEST, bn(0), bn(0))
+
+        const rsrBalInDestination = await rsr.balanceOf(other.address)
+        await rsr.connect(owner).mint(rsrTrader.address, issueAmount)
+        await rsrTrader.distributeTokenToBuy()
+        const expectedAmount = rsrBalInDestination.add(issueAmount)
+        expect(await rsr.balanceOf(other.address)).to.be.closeTo(expectedAmount, 100)
+      })
+
       it('Should claim but not sweep rewards to BackingManager from the Revenue Traders', async () => {
         rewardAmountAAVE = bn('0.5e18')
 
@@ -3500,7 +3534,13 @@ describe(`Revenues - P${IMPLEMENTATION}`, () => {
 
               await router.connect(addr1).bid(trade.address, addr1.address)
               expect(await trade.bidder()).to.equal(router.address)
-              // Cannot bid once is settled
+
+              // Noone can bid again on the trade directly
+              await expect(
+                trade.connect(addr1).bidWithCallback(new Uint8Array(0))
+              ).to.be.revertedWith('bid already received')
+
+              // Cannot bid once is settled via router
               await expect(
                 router.connect(addr1).bid(trade.address, addr1.address)
               ).to.be.revertedWith('trade not open')