Reserve contest - 0xA5DF's results

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/mixins/RecollateralizationLib.sol#L152-L202

Vulnerability details

Impact

The BackingManager.manageTokens() function checks if there's any deficit in collateral, in case there is, if there's a surplus from another collateral token it trades it to cover the deficit, otherwise it goes for a 'haircut' and cuts the amount of basket 'needed' (i.e. the number of baskets RToken claims to hold). In order to determine how much deficit/surplus there is the protocol calculates the 'basket range', where the top range is the optimistic estimation of the number of baskets the token would hold after trading and the bottom range is a pessimistic estimation.

The estimation is done by dividing the total collateral value by the price of 1 basket unit (for optimistic estimation the max value is divided by min price of basket-unit and vice versa). The problem is that this estimation is inefficient, for cases where just a little bit of collateral is missing the range 'band' (range.top - range.bottom) would be about 4% (when oracle error deviation is ±1%) instead of less than 1%.

This can cause the protocol an unnecessary haircut of a few percent where the deficit can be solved by simple trading.

This would also cause the price of RTokenAsset to deviate more than necessary before the haircut.

Proof of Concept

In the following PoC, the basket changed so that it has 99% of the required collateral for 3 tokens and 95% for the 4th. The basket range should be 98±0.03% (the basket has 95% collateral + 4% of 3/4 tokens. That 4% is worth 3±0.03% if we account for oracle error of their prices), but in reality the protocol calculates it as ~97.9±2%. That range causes the protocol to avoid trading and go to an unnecessary haircut to ~95%

diff --git a/contracts/plugins/assets/RTokenAsset.sol b/contracts/plugins/assets/RTokenAsset.sol
index 62223442..03d3c3f4 100644
--- a/contracts/plugins/assets/RTokenAsset.sol
+++ b/contracts/plugins/assets/RTokenAsset.sol
@@ -123,7 +123,7 @@ contract RTokenAsset is IAsset {
     // ==== Private ====
 
     function basketRange()
-        private
+        public
         view
         returns (RecollateralizationLibP1.BasketRange memory range)
     {
diff --git a/test/Recollateralization.test.ts b/test/Recollateralization.test.ts
index 3c53fa30..386c0673 100644
--- a/test/Recollateralization.test.ts
+++ b/test/Recollateralization.test.ts
@@ -234,7 +234,42 @@ describe(`Recollateralization - P${IMPLEMENTATION}`, () => {
         // Issue rTokens
         await rToken.connect(addr1)['issue(uint256)'](issueAmount)
       })
+      it('PoC basket range', async () => {
+        let range = await rTokenAsset.basketRange();
+        let basketTokens = await basketHandler.basketTokens();
+        console.log({range}, {basketTokens});
+        // Change the basket so that current balance would be 99 or 95 percent of
+        // the new basket
+        let q99PercentLess = 0.25 / 0.99;
+        let q95ercentLess = 0.25 / 0.95;
+        await basketHandler.connect(owner).setPrimeBasket(basketTokens, [fp(q99PercentLess),fp(q99PercentLess), fp(q95ercentLess), fp(q99PercentLess)])
+        await expect(basketHandler.connect(owner).refreshBasket())
+        .to.emit(basketHandler, 'BasketSet')
+
+        expect(await basketHandler.status()).to.equal(CollateralStatus.SOUND)
+        expect(await basketHandler.fullyCollateralized()).to.equal(false)
+
+        range = await rTokenAsset.basketRange();
+
+        // show the basket range is 95.9 to 99.9
+        console.log({range});
 
+        let needed = await rToken.basketsNeeded();
+
+        // show that prices are more or less the same
+        let prices = await Promise.all( basket.map(x => x.price()));
+
+        // Protocol would do a haircut even though it can easily do a trade
+        await backingManager.manageTokens([]);
+
+        // show how many baskets are left after the haircut
+         needed = await rToken.basketsNeeded();
+         
+        console.log({prices, needed});
+        return;
+    
+      })
+      return;
       it('Should select backup config correctly - Single backup token', async () => {
         // Register Collateral
         await assetRegistry.connect(owner).register(backupCollateral1.address)
@@ -602,7 +637,7 @@ describe(`Recollateralization - P${IMPLEMENTATION}`, () => {
         expect(quotes).to.eql([initialQuotes[0], initialQuotes[1], initialQuotes[3], bn('0.25e18')])
       })
     })
-
+    return;
     context('With multiple targets', function () {
       let issueAmount: BigNumber
       let newEURCollateral: FiatCollateral
@@ -785,7 +820,7 @@ describe(`Recollateralization - P${IMPLEMENTATION}`, () => {
       })
     })
   })
-
+  return;
   describe('Recollateralization', function () {
     context('With very simple Basket - Single stablecoin', function () {
       let issueAmount: BigNumber

Output (comments are added by me):

{
  range: [
    top: BigNumber { value: "99947916501440267201" },  //  99.9 basket units
    bottom: BigNumber { value: "95969983506382791000" } // 95.9 basket units
  ]
}
{
  prices: [
    [
      BigNumber { value: "990000000000000000" },
      BigNumber { value: "1010000000000000000" }
    ],
    [
      BigNumber { value: "990000000000000000" },
      BigNumber { value: "1010000000000000000" }
    ],
    [
      BigNumber { value: "990000000000000000" },
      BigNumber { value: "1010000000000000000" }
    ],
    [
      BigNumber { value: "19800000000000000" },
      BigNumber { value: "20200000000000000" }
    ]
  ],
  needed: BigNumber { value: "94999999905000000094" } // basket units after haircut: 94.9
}

Recommended Mitigation Steps

Change the formula so that we first calculate the 'base' (i.e. the min amount of baskets the RToken can satisfy without trading):

base = basketsHeldBy(backingManager) // in the PoC's case it'd be 95
(diffLowValue, diffHighValue) = (0,0) 
for each collateral token:
    diff = collateralBalance - basketHandler.quantity(base) 
    (diffLowValue, diffHighValue) = diff * (priceLow, priceHigh)
addBasketsLow = diffLowValue / basketPriceHigh
addBasketHigh = diffHighValue / basketPriceLow
range.top = base + addBasketHigh
range.bottom = base + addBasketLow

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/plugins/assets/Asset.sol#L144-L145

Vulnerability details

Impact

Asset.lotPrice() has a fallback mechanism in case that tryPrice() fails - it uses the last saved price and multiplies its value by lotMultiplier (a variable that decreases as the time since the last saved price increase) and returns the results. However, the tryPrice() might fail due to oracle timeout, in that case the last saved price might be older than the oracle's price.

This can cause the backing manager to misestimate the value of the asset, trade it at a lower price, or do an unnecessary haircut.

Proof of Concept

In the PoC below:

• Oracle price is set at day 0
• The asset is refreshed (e.g. somebody issued/vested/redeemed)
• After 5 days the oracle gets an update
• 25 hours later the lotPrice() is calculated based on the oracle price from day 0 even though a price from day 5 is available from the oracle
• Oracle gets another update
• 25 hours later the lotPrice() goes down to zero since it considers the price from day 0 (which is more than a week ago) to be the last saved price, even though a price from a day ago is available from the oracle

diff --git a/test/fixtures.ts b/test/fixtures.ts
index 5299a5f6..75ca8010 100644
--- a/test/fixtures.ts
+++ b/test/fixtures.ts
@@ -69,7 +69,7 @@ export const SLOW = !!useEnv('SLOW')
 
 export const PRICE_TIMEOUT = bn('604800') // 1 week
 
-export const ORACLE_TIMEOUT = bn('281474976710655').div(2) // type(uint48).max / 2
+export const ORACLE_TIMEOUT = bn('86400') // one day
 
 export const ORACLE_ERROR = fp('0.01') // 1% oracle error
 
diff --git a/test/plugins/Asset.test.ts b/test/plugins/Asset.test.ts
index d49c53f3..7f2f721e 100644
--- a/test/plugins/Asset.test.ts
+++ b/test/plugins/Asset.test.ts
@@ -233,6 +233,45 @@ describe('Assets contracts #fast', () => {
       )
     })
 
+    it('PoC lot price doesn\'t use most recent price', async () => {
+      // Update values in Oracles to 0
+
+      await setOraclePrice(rsrAsset.address, bn('1.1e8'))
+
+      await rsrAsset.refresh();
+      let [lotLow, lotHigh] = await rsrAsset.lotPrice();
+      let descripion = "day 0";
+      console.log({descripion, lotLow, lotHigh});
+      let hour = 60*60;
+      let day = hour*24;
+      await advanceTime(day * 5);
+
+      await setOraclePrice(rsrAsset.address, bn('2e8'));
+      // await rsrAsset.refresh();
+
+      [lotLow, lotHigh] = await rsrAsset.lotPrice();
+      descripion = 'after 5 days (right after update)';
+      console.log({descripion,lotLow, lotHigh});
+
+      await advanceTime(day + hour);
+
+      // Fallback prices should be zero
+
+      [lotLow, lotHigh] = await rsrAsset.lotPrice();
+      descripion = 'after 6+ days';
+      console.log({descripion, lotLow, lotHigh});
+
+      await setOraclePrice(rsrAsset.address, bn('2e8'));
+
+      await advanceTime(day + hour);
+
+      [lotLow, lotHigh] = await rsrAsset.lotPrice();
+      descripion = 'after 7+ days';
+      console.log({descripion, lotLow, lotHigh});
+
+    })
+    return;
+
     it('Should return (0, 0) if price is zero', async () => {
       // Update values in Oracles to 0
       await setOraclePrice(compAsset.address, bn('0'))
@@ -595,6 +634,7 @@ describe('Assets contracts #fast', () => {
       expect(lotHighPrice4).to.be.equal(bn(0))
     })
   })
+  return;
 
   describe('Constructor validation', () => {
     it('Should not allow price timeout to be zero', async () => {

Output:

{
  descripion: 'day 0',
  lotLow: BigNumber { value: "1089000000000000000" },
  lotHigh: BigNumber { value: "1111000000000000000" }
}
{
  descripion: 'after 5 days (right after update)',
  lotLow: BigNumber { value: "1980000000000000000" },
  lotHigh: BigNumber { value: "2020000000000000000" }
}
{
  descripion: 'after 6+ days',
  lotLow: BigNumber { value: "149087485119047618" },
  lotHigh: BigNumber { value: "152099353505291005" }
}
{
  descripion: 'after 7+ days', // `lotPrice()` returns zero even though the most recent price the oracle holds is from 25 hours ago
  lotLow: BigNumber { value: "0" },
  lotHigh: BigNumber { value: "0" }
}

Recommended Mitigation Steps

Allow specifying a timeout to tryPrice(), in case that tryPrice() fails due to oracle timeout then call it again with priceTimeout as the timeout. If the call succeeds the second time then use it as the most recent price for fallback calculations.

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/plugins/assets/Asset.sol#L102 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/plugins/assets/FiatCollateral.sol#L149 https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/plugins/assets/OracleLib.sol#L14-L31

Vulnerability details

The Asset.refresh() function calls tryPrice() and catches all errors except errors with empty data. As explained in the docs the reason empty errors aren't caught is in order to prevent an attacker from failing the tryPrice() intentionally by running it out of gas. However, an error with empty data isn't thrown only in case of out of gas, in the current way that Chainlink deprecates oracles (by setting aggregator to the zero address) a deprecated oracle would also throw an empty error.

Impact

Any function that requires refreshing the assets will fail to execute (till the asset is replaced in the asset registry, passing the proposal via governance would usually take 7 days), that includes:

• Issuance
• Vesting
• Redemption
• Auctions (manageTokens())
• StRSR.withdraw()

Proof of Concept

The docs imply in case of deprecation the protocol is expected continue to operate:

If an asset's oracle goes offline forever, its lotPrice() will eventually reach [0, 0] and the protocol will completely stop trading this asset.

The docs also clearly state that 'refresh() should never revert'

I've tracked a few Chainlink oracles that were deprecated on the Polygon network on Jan 11, the PoC below tests an Asset.refresh() call with a deprecated oracle.

File: test/plugins/Deprecated.test.ts

import { Wallet, ContractFactory } from 'ethers'
import { ethers, network, waffle } from 'hardhat'
import { IConfig } from '../../common/configuration'
import { bn, fp } from '../../common/numbers'
import {
  Asset,
  ATokenFiatCollateral,
  CTokenFiatCollateral,
  CTokenMock,
  ERC20Mock,
  FiatCollateral,
  IAssetRegistry,
  RTokenAsset,
  StaticATokenMock,
  TestIBackingManager,
  TestIRToken,
  USDCMock,
} from '../../typechain'
import {
  Collateral,
  defaultFixture,
} from '../fixtures'

const createFixtureLoader = waffle.createFixtureLoader


describe('Assets contracts #fast', () => {
  // Tokens
  let rsr: ERC20Mock
  let compToken: ERC20Mock
  let aaveToken: ERC20Mock
  let rToken: TestIRToken
  let token: ERC20Mock
  let usdc: USDCMock
  let aToken: StaticATokenMock
  let cToken: CTokenMock

  // Assets
  let collateral0: FiatCollateral
  let collateral1: FiatCollateral
  let collateral2: ATokenFiatCollateral
  let collateral3: CTokenFiatCollateral

  // Assets
  let rsrAsset: Asset
  let compAsset: Asset
  let aaveAsset: Asset
  let rTokenAsset: RTokenAsset
  let basket: Collateral[]

  // Config
  let config: IConfig

  // Main
  let loadFixture: ReturnType<typeof createFixtureLoader>
  let wallet: Wallet
  let assetRegistry: IAssetRegistry
  let backingManager: TestIBackingManager

  // Factory
  let AssetFactory: ContractFactory
  let RTokenAssetFactory: ContractFactory

  const amt = fp('1e4')

  before('create fixture loader', async () => {
    ;[wallet] = (await ethers.getSigners()) as unknown as Wallet[]
    loadFixture = createFixtureLoader([wallet])
  })

  beforeEach(async () => {
    // Deploy fixture
    ;({
      rsr,
      rsrAsset,
      compToken,
      compAsset,
      aaveToken,
      aaveAsset,
      basket,
      assetRegistry,
      backingManager,
      config,
      rToken,
      rTokenAsset,
    } = await loadFixture(defaultFixture))

    // Get collateral tokens
    collateral0 = <FiatCollateral>basket[0]
    collateral1 = <FiatCollateral>basket[1]
    collateral2 = <ATokenFiatCollateral>basket[2]
    collateral3 = <CTokenFiatCollateral>basket[3]
    token = <ERC20Mock>await ethers.getContractAt('ERC20Mock', await collateral0.erc20())
    usdc = <USDCMock>await ethers.getContractAt('USDCMock', await collateral1.erc20())
    aToken = <StaticATokenMock>(
      await ethers.getContractAt('StaticATokenMock', await collateral2.erc20())
    )
    cToken = <CTokenMock>await ethers.getContractAt('CTokenMock', await collateral3.erc20())

    await rsr.connect(wallet).mint(wallet.address, amt)
    await compToken.connect(wallet).mint(wallet.address, amt)
    await aaveToken.connect(wallet).mint(wallet.address, amt)

    // Issue RToken to enable RToken.price
    for (let i = 0; i < basket.length; i++) {
      const tok = await ethers.getContractAt('ERC20Mock', await basket[i].erc20())
      await tok.connect(wallet).mint(wallet.address, amt)
      await tok.connect(wallet).approve(rToken.address, amt)
    }
    await rToken.connect(wallet)['issue(uint256)'](amt)

    AssetFactory = await ethers.getContractFactory('Asset')
    RTokenAssetFactory = await ethers.getContractFactory('RTokenAsset')
  })

  describe('Deployment', () => {
    it('Deployment should setup assets correctly', async () => {

        console.log(network.config.chainId);
        // let validOracle = '0x443C5116CdF663Eb387e72C688D276e702135C87';
        let deprecatedOracle = '0x2E5B04aDC0A3b7dB5Fd34AE817c7D0993315A8a6';
        let priceTimeout_ = await aaveAsset.priceTimeout(),
         chainlinkFeed_ = deprecatedOracle,
         oracleError_ = await aaveAsset.oracleError(),
         erc20_ = await aaveAsset.erc20(),
         maxTradeVolume_ = await aaveAsset.maxTradeVolume(),
         oracleTimeout_ = await aaveAsset.oracleTimeout();
        
        aaveAsset = await AssetFactory.deploy(priceTimeout_,
            chainlinkFeed_ ,
            oracleError_ ,
            erc20_ ,
            maxTradeVolume_ ,
            oracleTimeout_  ) as Asset;

        await aaveAsset.refresh();
      
    })
  })

})

Modification of hardhat.config.ts to set it to the Polygon network:

diff --git a/hardhat.config.ts b/hardhat.config.ts
index f1886d25..53565799 100644
--- a/hardhat.config.ts
+++ b/hardhat.config.ts
@@ -24,18 +24,19 @@ const TIMEOUT = useEnv('SLOW') ? 3_000_000 : 300_000
 const src_dir = `./contracts/${useEnv('PROTO')}`
 const settings = useEnv('NO_OPT') ? {} : { optimizer: { enabled: true, runs: 200 } }
 
+let recentBlockNumber = 38231040;
+let jan6Block = 37731612; // causes 'missing trie node' error
+
 const config: HardhatUserConfig = {
   defaultNetwork: 'hardhat',
   networks: {
     hardhat: {
       // network for tests/in-process stuff
-      forking: useEnv('FORK')
-        ? {
-            url: MAINNET_RPC_URL,
-            blockNumber: Number(useEnv('MAINNET_BLOCK', forkBlockNumber['default'].toString())),
-          }
-        : undefined,
-      gas: 0x1ffffffff,
+      forking: {
+          url: "https://rpc.ankr.com/polygon",
+          // blockNumber: recentBlockNumber
+          },
+          gas: 0x1ffffffff,
       blockGasLimit: 0x1fffffffffffff,
       allowUnlimitedContractSize: true,
     },

Output:

  1) Assets contracts #fast
       Deployment
         Deployment should setup assets correctly:
     Error: Transaction reverted without a reason string
    at Asset.refresh (contracts/plugins/assets/Asset.sol:102)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async HardhatNode._mineBlockWithPendingTxs (node_modules/hardhat/src/internal/hardhat-network/provider/node.ts:1802:23)
    at async HardhatNode.mineBlock (node_modules/hardhat/src/internal/hardhat-network/provider/node.ts:491:16)
    at async EthModule._sendTransactionAndReturnHash (node_modules/hardhat/src/internal/hardhat-network/provider/modules/eth.ts:1522:18)
    at async HardhatNetworkProvider.request (node_modules/hardhat/src/internal/hardhat-network/provider/provider.ts:118:18)
    at async EthersProviderWrapper.send (node_modules/@nomiclabs/hardhat-ethers/src/internal/ethers-provider-wrapper.ts:13:20)

Notes:

• Chainlink list deprecating oracles only till deprecation, afterwards they're removed from the website. For this reason I wasn't able to trace deprecated oracles on the mainnet
• I was trying to prove this worked before deprecation, however, I kept getting the 'missing trie node' error when forking the older block. This isn't essential for the PoC so I decided to give up on it for now (writing this PoC was hard enough on its own).

Recommended Mitigation Steps

At OracleLib.price() catch the error and check if the error data is empty and the aggregator is set to the zero address, if it is a revert with a custom error. Otherwise revert with the original error data (this can be done with assembly). Another approach might be to check in the refresh() function that the tryPrice() function didn't revert due to out of gas error by checking the gas before and after (in case of out of gas error only ~1/64 of the gas-before would be left). The advantage of this approach is that it would catch also other errors that might revert with empty data.

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L560-L564

Vulnerability details

Impact

Users who wish to unstake their RSR from StRSR have to first unstake and then wait unstakingDelay till they can actually withdraw their stake. The unstakingDelay can change by the governance. The issue is that when the unstakingDelay is decreased - users that have pending unstakes (aka drafts) would have to wait till the old delay has passed for the pending draft (not only for their pending drafts, but also for any new draft they wish to create. e.g. if the unstaking delay was 6 months and was changed to 2 weeks, if a user has a pending draft that was created a month before the change the user would have to wait at least 5 months since the change for every new draft).

Proof of Concept

The following PoC shows an example similar to above:

• Unstaking delay was 6 months
• Bob unstaked (create a draft) 1 wei of RSR
• Unstaking delay was changed to 2 weeks
• Both Bob and Alice unstake their remaining stake
• Alice can withdraw her stake after 2 weeks
• Bob has to wait 6 months in order to withdraw both that 1 wei and the remaining of the stake

diff --git a/test/ZZStRSR.test.ts b/test/ZZStRSR.test.ts
index f507cd50..3312686a 100644
--- a/test/ZZStRSR.test.ts
+++ b/test/ZZStRSR.test.ts
@@ -599,6 +599,8 @@ describe(`StRSRP${IMPLEMENTATION} contract`, () => {
       let amount2: BigNumber
       let amount3: BigNumber
 
+      let sixMonths = bn(60*60*24*30*6);
+
       beforeEach(async () => {
         stkWithdrawalDelay = bn(await stRSR.unstakingDelay()).toNumber()
 
@@ -608,18 +610,56 @@ describe(`StRSRP${IMPLEMENTATION} contract`, () => {
         amount3 = bn('3e18')
 
         // Approve transfers
-        await rsr.connect(addr1).approve(stRSR.address, amount1)
+        await rsr.connect(addr1).approve(stRSR.address, amount1.add(1))
         await rsr.connect(addr2).approve(stRSR.address, amount2.add(amount3))
 
+        
         // Stake
-        await stRSR.connect(addr1).stake(amount1)
+        await stRSR.connect(addr1).stake(amount1.add(1))
         await stRSR.connect(addr2).stake(amount2)
         await stRSR.connect(addr2).stake(amount3)
 
-        // Unstake - Create withdrawal
-        await stRSR.connect(addr1).unstake(amount1)
+        // here
+        let sixMonths = bn(60*60*24*30*6);
+        // gov thinks it's a good idea to set delay to 6 months
+        await expect(stRSR.connect(owner).setUnstakingDelay(sixMonths))
+        .to.emit(stRSR, 'UnstakingDelaySet')
+        .withArgs(config.unstakingDelay, sixMonths);
+
+        // Poor Bob created a draft when unstaking delay was 6 months
+        await stRSR.connect(addr1).unstake(bn(1))
+
+        // gov revise their previous decision and set unstaking delay back to 2 weeks
+        await expect(stRSR.connect(owner).setUnstakingDelay(config.unstakingDelay))
+        .to.emit(stRSR, 'UnstakingDelaySet')
+        .withArgs(sixMonths, config.unstakingDelay);
+
+        // now both Bob and Alice decide to unstake
+        await stRSR.connect(addr1).unstake(amount1);
+        await stRSR.connect(addr2).unstake(amount2);
+
+      })
+
+      it('PoC user 1 can\'t withdraw', async () => {
+        // Get current balance for user
+        const prevAddr1Balance = await rsr.balanceOf(addr1.address)
+
+        // 6 weeks have passed, much more than current delay
+        await advanceTime(stkWithdrawalDelay * 3)
+
+
+        // Alice can happily withdraw her stake
+        await stRSR.connect(addr2).withdraw(addr2.address, 1)
+        // Bob can't withdraw his stake and has to wait 6 months
+        // Bob is now very angry and wants to talk to the manager
+        await expect(stRSR.connect(addr1).withdraw(addr1.address, 2)).to.be.revertedWith(
+          'withdrawal unavailable'
+        )
+
+
       })
 
+      return; // don't run further test
       it('Should revert withdraw if Main is paused', async () => {
         // Get current balance for user
         const prevAddr1Balance = await rsr.balanceOf(addr1.address)
@@ -1027,6 +1067,7 @@ describe(`StRSRP${IMPLEMENTATION} contract`, () => {
       })
     })
   })
+  return; // don't run further test
 
   describe('Add RSR / Rewards', () => {
     const initialRate = fp('1')

Recommended Mitigation Steps

Allow users to use current delay even if it was previously higher. I think this should apply not only to new drafts but also for drafts that were created before the change. Alternatively, the protocol can set a rule that even if the staking delay was lowered stakers have to wait at least the old delay since the change till they can withdraw. But in this case the rule should apply to everybody regardless if they have pending drafts or not.

Div before multiply at Distributor.distribute()

Here there's a div before multiplication distribution that might cause the amount distributed to be lower than it should. This would mean that value up to the amount of total shares (1e7 wei) can be locked in the sender without being able to distribute it.

Cache in memory storage variable / view-calls that are read multiple times

Gas saving: ~700 gas units in total

Code: /contracts/p0/RToken.sol#L466-L473

blockIssuanceRates[block.number] is read twice (and written once). It can be cached to a memory variable to save gas (and the memory variable should be updated too if the storage variable is changed).

        // Calculate the issuance rate if this is the first issue in the block
        if (blockIssuanceRates[block.number] == 0) {
            blockIssuanceRates[block.number] = Math.max(
                MIN_BLOCK_ISSUANCE_LIMIT,
                issuanceRate.mulu_toUint(totalSupply())
            );
        }
        uint256 perBlock = blockIssuanceRates[block.number];

Code: /contracts/p1/AssetRegistry.sol#L165-L166

assets[erc20] is being read twice here:

            if (assets[erc20] == asset) return false;
            else emit AssetUnregistered(erc20, assets[erc20]);

Code: /contracts/p1/BackingManager.sol#L139-L142

req.sell.erc20() is used read here twice if the condition is true

                if (req.sell.erc20() == rsr) {
                    uint256 bal = req.sell.erc20().balanceOf(address(this));
                    if (req.sellAmount > bal) stRSR.seizeRSR(req.sellAmount - bal);

Code: /contracts/p1/StRSR.sol#L497-L498

payoutLastPaid and rewardPeriod are both storage variable that are read twice here

        if (block.timestamp < payoutLastPaid + rewardPeriod) return;
        uint48 numPeriods = (uint48(block.timestamp) - payoutLastPaid) / rewardPeriod; 

Code: /contracts/plugins/trading/GnosisTrade.sol#L174-L192

Here origin is being read 3 times. In the 2 last times msg.sender can be used instead, since we've verified they're equal (msg.sender costs 2 gas units, while reading a storage variable costs 100 gas units).

        require(msg.sender == origin, "only origin can settle");

        // Optionally process settlement of the auction in Gnosis
        if (!isAuctionCleared()) {
            // By design, we don't rely on this return value at all, just the
            // "cleared" state of the auction, and the token balances this contract owns.
            // slither-disable-next-line unused-return
            gnosis.settleAuction(auctionId);
            assert(isAuctionCleared());
        }

        // At this point we know the auction has cleared

        // Transfer balances to origin
        uint256 sellBal = sell.balanceOf(address(this));
        boughtAmt = buy.balanceOf(address(this));

        if (sellBal > 0) IERC20Upgradeable(address(sell)).safeTransfer(origin, sellBal);
        if (boughtAmt > 0) IERC20Upgradeable(address(buy)).safeTransfer(origin, boughtAmt);

Convert to fix and divide instead of using divuu()

Gas saving: ~100-200 gas units

Code: /contracts/plugins/assets/Asset.sol#L141-L142

divuu() is a pretty expensive function, it uses mulDiv256() which uses about 40-50 math operations (should cost 100-200 gas units, for some reason the gas tests don't report it). Since priceTimeout - delta is unit48 anyways, it can be converted to a fixed and then divide using divu().

-            uint192 lotMultiplier = divuu(priceTimeout - delta, priceTimeout);
+            uint192 lotMultiplier = toFix(priceTimeout - delta).divu(priceTimeout);