Wrong gap counting algorithm is used in upgradable contracts

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/AssetRegistry.sol#L184
https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/BasketHandler.sol#L738
https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/Distributor.sol#L190
https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/RToken.sol#L845
https://github.com/reserve-protocol/protocol/blob/df7ecadc2bae74244ace5e8b39e94bc992903158/contracts/p1/StRSR.sol#L839

Vulnerability details

Impact

The developer used a wrong algorithm to count the state variables of struct types.
This isn't an accident, so it's likely that contracts won't work in a future upgrade, or even that most contracts wll be in a wrong state and can't be upgraded again, leaving all assets locked.

Proof of Concept

The following table lists the gap information of all upgradable contracts (the code used to print this table can be found at the end of this section):

Contract	Y/N	GapOffset	GapSize	CorrectGap
AssetRegistryP1	N	4	47	46
Auth	Y	2	48	48
BackingManagerP1	Y	9	41	41
BasketHandlerP1	N	16	42	34
BrokerP1	Y	6	44	44
ComponentP1	Y	1	49	49
ComponentRegistry	Y	10	40	40
DistributorP1	N	7	46	43
ERC20PermitUpgradeable	Y	2	48	48
FurnaceP1	Y	3	47	47
MainP1	Y	1	49	49
RTokenP1	N	15	37	35
RevenueTraderP1	Y	3	47	47
StRSRP1	N	22	30	28
StRSRP1Votes	Y	4	46	46
TradingP1	Y	4	46	46

There are 5 contracts that calculate the gap size incorrectly.

Let's analyze how these gap calculation errors are caused one by one:

• AssetRegistryP1: Wrong count for _erc20s

  EnumerableSet.AddressSet private _erc20s;
  

• BasketHandlerP1: Wrong count for config and basket, missed _targetNames and _newBasket

  BasketConfig private config;
  Basket private basket;
  

• DistributorP1: Missed destinations and distribution
• RTokenP1: Wrong count for battery

  RedemptionBatteryLib.Battery private battery;
  

• StRSRP1: Missed variables of 2 slots

From the above, we can find that only one slot is counted for any struct type state variable.
This counting algorithm is wrong, a state variables of struct type needs to be expanded to count the slots.

In a proxy contract upgrade, if the slot calculation of a sub-contract is wrong, the state of the upgraded contract may be confused due to the storage slots shift, which may cause the contracts not work or work abnormally, and even may never be able to upgrade again.

For example, suppose that a future update replaces the ContextUpgradeable with a custom one that modifies the ContextUpgradeable as follows:

abstract contract ContextUpgradeable is Initializable {
    // Add a new state variable
    EnumerableSet.AddressSet private _erc20s;
    ...
    // Update gap: 50 -> 49
    uint256[49] private __gap;
}

The wrong gap calculation in this contract is the same as in the previous contracts(AssetRegistryP1, BasketHandlerP1, etc).
The correct gap size should be 48.

This update will cause serious problems after upgrading, almost all the contracts will work abnormally and never be able to updatable again, such as: AssetRegistry, BasketHandler, Broker, Distributor, Furnace, RToken, StRSR, RevenueTrader, etc.

This is because all these contracts inherit ContextUpgradeable through Component.
The modification of ContextUpgradeable changes its head slot size from 50 to 51, which causes all the state variables of its subcontracts to be shifted by 1 slot.
As a result, all the contracts will get into a state mess and not work properly.

To make matters worse, the shift makes the variable Component.main, which is required for upgrading, point to a wrong address. All calls to main will fail.

All assets will be locked up in the contracts:

• RToken.redeem() will revert because the its modifier notFrozen involves the main:

  modifier notFrozen() {
      require(!main.frozen(), "frozen");
      _;
  }
  

• StRSR.unstake() will revert because its modifier notPausedOrFrozen involves the main:

  modifier notPausedOrFrozen() {
      require(!main.pausedOrFrozen(), "paused or frozen");
      _;
  }
  

• etc

Also, there is no way to fix this problem because all upgrades will revert by the modifier governance at Component._authorizeUpgrade() for the same reason:

abstract contract ComponentP1 is
    Versioned,
    Initializable,
    ContextUpgradeable,
    UUPSUpgradeable,
    IComponent
{
    ...
    modifier governance() {
        // WILL REVERT FOR THE WRONG MAIN ADDRESS
        require(main.hasRole(OWNER, _msgSender()), "governance only");
        _;
    }
    ...
    function _authorizeUpgrade(address newImplementation) internal view override governance {}
    ...
}

Code used to print the gap table:

diff --git a/contracts/mixins/Auth.sol b/contracts/mixins/Auth.sol
index 1f4502d9..ff6c4578 100644
--- a/contracts/mixins/Auth.sol
+++ b/contracts/mixins/Auth.sol
@@ -205,4 +205,11 @@ abstract contract Auth is AccessControlUpgradeable, IAuth {
      * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
      */
     uint256[48] private __gap;
+
+    function gapOfAuth() public view returns (uint256 gapOffset, uint256 gapSize) {
+        assembly {
+            gapOffset := sub(__gap.slot, longFreezes.slot)
+        }
+        gapSize = __gap.length;
+    }
 }
diff --git a/contracts/mixins/ComponentRegistry.sol b/contracts/mixins/ComponentRegistry.sol
index 5455f541..5bd3f876 100644
--- a/contracts/mixins/ComponentRegistry.sol
+++ b/contracts/mixins/ComponentRegistry.sol
@@ -117,4 +117,11 @@ abstract contract ComponentRegistry is Initializable, Auth, IComponentRegistry {
      * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
      */
     uint256[40] private __gap;
+
+    function gapOfComponentRegistry() public view returns (uint256 gapOffset, uint256 gapSize) {
+        assembly {
+            gapOffset := sub(__gap.slot, rToken.slot)
+        }
+        gapSize = __gap.length;
+    }
 }
diff --git a/contracts/p1/AssetRegistry.sol b/contracts/p1/AssetRegistry.sol
index 18ef1eb1..f0b1e508 100644
--- a/contracts/p1/AssetRegistry.sol
+++ b/contracts/p1/AssetRegistry.sol
@@ -182,4 +182,11 @@ contract AssetRegistryP1 is ComponentP1, IAssetRegistry {
      * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
      */
     uint256[47] private __gap;
+
+    function gapOfAssetRegistryP1() public view returns (uint256 gapOffset, uint256 gapSize) {
+        assembly {
+            gapOffset := sub(__gap.slot, basketHandler.slot)
+        }
+        gapSize = __gap.length;
+    }
 }
diff --git a/contracts/p1/BackingManager.sol b/contracts/p1/BackingManager.sol
index 431e0796..087bda83 100644
--- a/contracts/p1/BackingManager.sol
+++ b/contracts/p1/BackingManager.sol
@@ -272,4 +272,11 @@ contract BackingManagerP1 is TradingP1, IBackingManager {
      * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
      */
     uint256[41] private __gap;
+
+    function gapOfBackingManagerP1() public view returns (uint256 gapOffset, uint256 gapSize) {
+        assembly {
+            gapOffset := sub(__gap.slot, assetRegistry.slot)
+        }
+        gapSize = __gap.length;
+    }
 }
diff --git a/contracts/p1/BasketHandler.sol b/contracts/p1/BasketHandler.sol
index f74155b1..284a9243 100644
--- a/contracts/p1/BasketHandler.sol
+++ b/contracts/p1/BasketHandler.sol
@@ -736,4 +736,11 @@ contract BasketHandlerP1 is ComponentP1, IBasketHandler {
      * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
      */
     uint256[42] private __gap;
+
+    function gapOfBasketHandlerP1() public view returns (uint256 gapOffset, uint256 gapSize) {
+        assembly {
+            gapOffset := sub(__gap.slot, assetRegistry.slot)
+        }
+        gapSize = __gap.length;
+    }
 }
diff --git a/contracts/p1/Broker.sol b/contracts/p1/Broker.sol
index 35c2423b..c59b1482 100644
--- a/contracts/p1/Broker.sol
+++ b/contracts/p1/Broker.sol
@@ -151,4 +151,11 @@ contract BrokerP1 is ComponentP1, IBroker {
      * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
      */
     uint256[44] private __gap;
+
+    function gapOfBrokerP1() public view returns (uint256 gapOffset, uint256 gapSize) {
+        assembly {
+            gapOffset := sub(__gap.slot, backingManager.slot)
+        }
+        gapSize = __gap.length;
+    }
 }
diff --git a/contracts/p1/Distributor.sol b/contracts/p1/Distributor.sol
index 53322973..d591c023 100644
--- a/contracts/p1/Distributor.sol
+++ b/contracts/p1/Distributor.sol
@@ -188,4 +188,11 @@ contract DistributorP1 is ComponentP1, IDistributor {
      * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
      */
     uint256[46] private __gap;
+
+    function gapOfDistributorP1() public view returns (uint256 gapOffset, uint256 gapSize) {
+        assembly {
+            gapOffset := sub(__gap.slot, destinations.slot)
+        }
+        gapSize = __gap.length;
+    }
 }
diff --git a/contracts/p1/Furnace.sol b/contracts/p1/Furnace.sol
index fcc63605..80b5ef02 100644
--- a/contracts/p1/Furnace.sol
+++ b/contracts/p1/Furnace.sol
@@ -106,4 +106,11 @@ contract FurnaceP1 is ComponentP1, IFurnace {
      * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
      */
     uint256[47] private __gap;
+
+    function gapOfFurnaceP1() public view returns (uint256 gapOffset, uint256 gapSize) {
+        assembly {
+            gapOffset := sub(__gap.slot, ratio.slot)
+        }
+        gapSize = __gap.length;
+    }
 }
diff --git a/contracts/p1/Main.sol b/contracts/p1/Main.sol
index ca5b57ec..d78813b2 100644
--- a/contracts/p1/Main.sol
+++ b/contracts/p1/Main.sol
@@ -69,4 +69,11 @@ contract MainP1 is Versioned, Initializable, Auth, ComponentRegistry, UUPSUpgrad
      * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
      */
     uint256[49] private __gap;
+
+    function gapOfMainP1() public view returns (uint256 gapOffset, uint256 gapSize) {
+        assembly {
+            gapOffset := sub(__gap.slot, rsr.slot)
+        }
+        gapSize = __gap.length;
+    }
 }
diff --git a/contracts/p1/RToken.sol b/contracts/p1/RToken.sol
index 2420c8d6..bccf3c8d 100644
--- a/contracts/p1/RToken.sol
+++ b/contracts/p1/RToken.sol
@@ -843,4 +843,11 @@ contract RTokenP1 is ComponentP1, ERC20PermitUpgradeable, IRToken {
      * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
      */
     uint256[37] private __gap;
+
+    function gapOfRTokenP1() public view returns (uint256 gapOffset, uint256 gapSize) {
+        assembly {
+            gapOffset := sub(__gap.slot, mandate.slot)
+        }
+        gapSize = __gap.length;
+    }
 }
diff --git a/contracts/p1/RevenueTrader.sol b/contracts/p1/RevenueTrader.sol
index e4c8013f..83ddac52 100644
--- a/contracts/p1/RevenueTrader.sol
+++ b/contracts/p1/RevenueTrader.sol
@@ -99,4 +99,11 @@ contract RevenueTraderP1 is TradingP1, IRevenueTrader {
      * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
      */
     uint256[47] private __gap;
+
+    function gapOfRevenueTraderP1() public view returns (uint256 gapOffset, uint256 gapSize) {
+        assembly {
+            gapOffset := sub(__gap.slot, tokenToBuy.slot)
+        }
+        gapSize = __gap.length;
+    }
 }
diff --git a/contracts/p1/StRSR.sol b/contracts/p1/StRSR.sol
index 8fe1c3e7..a01f13c3 100644
--- a/contracts/p1/StRSR.sol
+++ b/contracts/p1/StRSR.sol
@@ -837,4 +837,11 @@ abstract contract StRSRP1 is Initializable, ComponentP1, IStRSR, EIP712Upgradeab
      * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
      */
     uint256[30] private __gap;
+
+    function gapOfStRSRP1() public view returns (uint256 gapOffset, uint256 gapSize) {
+        assembly {
+            gapOffset := sub(__gap.slot, name.slot)
+        }
+        gapSize = __gap.length;
+    }
 }
diff --git a/contracts/p1/StRSRVotes.sol b/contracts/p1/StRSRVotes.sol
index 7070381a..566af851 100644
--- a/contracts/p1/StRSRVotes.sol
+++ b/contracts/p1/StRSRVotes.sol
@@ -225,4 +225,11 @@ contract StRSRP1Votes is StRSRP1, IStRSRVotes {
      * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
      */
     uint256[46] private __gap;
+
+    function gapOfStRSRP1Votes() public view returns (uint256 gapOffset, uint256 gapSize) {
+        assembly {
+            gapOffset := sub(__gap.slot, _delegates.slot)
+        }
+        gapSize = __gap.length;
+    }
 }
diff --git a/contracts/p1/mixins/Component.sol b/contracts/p1/mixins/Component.sol
index a72d6ac9..56fcb646 100644
--- a/contracts/p1/mixins/Component.sol
+++ b/contracts/p1/mixins/Component.sol
@@ -62,4 +62,11 @@ abstract contract ComponentP1 is
      * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
      */
     uint256[49] private __gap;
+
+    function gapOfComponentP1() public view returns (uint256 gapOffset, uint256 gapSize) {
+        assembly {
+            gapOffset := sub(__gap.slot, main.slot)
+        }
+        gapSize = __gap.length;
+    }
 }
diff --git a/contracts/p1/mixins/Trading.sol b/contracts/p1/mixins/Trading.sol
index 2ef15d17..5e131c49 100644
--- a/contracts/p1/mixins/Trading.sol
+++ b/contracts/p1/mixins/Trading.sol
@@ -155,4 +155,11 @@ abstract contract TradingP1 is Multicall, ComponentP1, ReentrancyGuardUpgradeabl
      * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
      */
     uint256[46] private __gap;
+
+    function gapOfTradingP1() public view returns (uint256 gapOffset, uint256 gapSize) {
+        assembly {
+            gapOffset := sub(__gap.slot, broker.slot)
+        }
+        gapSize = __gap.length;
+    }
 }
diff --git a/contracts/vendor/ERC20PermitUpgradeable.sol b/contracts/vendor/ERC20PermitUpgradeable.sol
index 8c9dc6f7..671f1132 100644
--- a/contracts/vendor/ERC20PermitUpgradeable.sol
+++ b/contracts/vendor/ERC20PermitUpgradeable.sol
@@ -125,4 +125,11 @@ abstract contract ERC20PermitUpgradeable is
      * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
      */
     uint256[48] private __gap;
+
+    function gapOfERC20PermitUpgradeable() public view returns (uint256 gapOffset, uint256 gapSize) {
+        assembly {
+            gapOffset := sub(__gap.slot, _nonces.slot)
+        }
+        gapSize = __gap.length;
+    }
 }
diff --git a/test/Deployer.test.ts b/test/Deployer.test.ts
index f8fb9362..fcf3ad6a 100644
--- a/test/Deployer.test.ts
+++ b/test/Deployer.test.ts
@@ -127,6 +127,37 @@ describe(`DeployerP${IMPLEMENTATION} contract #fast`, () => {
     } = await loadFixture(defaultFixture))
   })
 
+  it.only('Print gap storage info', async () => {
+    const items = [
+      {name: "AssetRegistryP1", addr: assetRegistry.address},
+      {name: "Auth", addr: main.address},
+      {name: "BackingManagerP1", addr: backingManager.address},
+      {name: "BasketHandlerP1", addr: basketHandler.address},
+      {name: "BrokerP1", addr: broker.address},
+      {name: "ComponentP1", addr: assetRegistry.address},
+      {name: "ComponentRegistry", addr: main.address},
+      {name: "DistributorP1", addr: distributor.address},
+      {name: "ERC20PermitUpgradeable", addr: rToken.address},
+      {name: "FurnaceP1", addr: furnace.address},
+      {name: "MainP1", addr: main.address},
+      {name: "RTokenP1", addr: rToken.address},
+      {name: "RevenueTraderP1", addr: rTokenTrader.address},
+      {name: "StRSRP1", addr: stRSR.address},
+      {name: "StRSRP1Votes", addr: stRSR.address},
+      {name: "TradingP1", addr: backingManager.address},
+    ]
+    console.log(`| ${"Contract".padEnd(22)} | Y/N | GapOffset | GapSize | CorrectGap |`);
+    for (let item of items) {
+      const c = await ethers.getContractAt(item.name, item.addr)
+      const info = await c[`gapOf${item.name}`]();
+      const isCorrect = info.gapOffset.add(info.gapSize).eq(50) ? 'Y' : 'N'
+      const gapOffset = info.gapOffset.toString().padEnd(9)
+      const gapSize = info.gapSize.toString().padEnd(7)
+      const correctGap = bn(50).sub(info.gapOffset).toString().padEnd(10)
+      console.log(`| ${item.name.padEnd(22)} |  ${isCorrect}  | ${gapOffset} | ${gapSize} | ${correctGap} |`)
+    }
+  });
+
   describe('Validations', () => {
     it('Should validate addresses in constructor', async () => {
       const validateComponent = async (

Tools Used

VS Code

Recommended Mitigation Steps

1. Code style

It is recommended to declare the gap rule, correct gap counting algorithm and emphasize the importance of correctness in the docs(such as solidity-style.md#storage-gaps).
It does not matter whether the head slot size (including the gap) of a contract is 50 or not. The important thing is that the calculation must be correct, and the head slot size must remain the same before and after each update.

2. Code modification

If this new version will be deployed directly instead of upgrading the old version, it is recommended that all gaps be re-calculated to meet the sum-50-slots rule.

Otherwise, it is recommended to add a uint256[x] __ignore_extra_gap; to the end of the wrong-gap contracts, where x is the head total size of the contract minus 50.
In this way, the number of lots occupied by regular variables and __gaps is 50 again for each contract, which can make future maintenance more convenient and safer.

3. Inspection tool

Consider developing a tool/task to veriry the gap size each time a contract is modified.
The head slot size before and after each update should be guaranteed to remain the same.

[0xean]

given that gaps aren't a strict requirement and more of a best practice to make things easier / less error prone, I am downgrading this issue to QA.

For more discussion on the topic see - code-423n4/org#55

[c4-judge]

Duplicate of #315