code423n4 commented 1 year ago

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/BasketHandler.sol#L53-L78 https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/BasketHandler.sol#L684

Vulnerability details

Impact

BasketHandlerP1 is not a subcontract of another contract, so this issue will not be raised in this upgrade. However, wrong slots calculation method may lead a contract not work after an upgrade in the future, it may be in a wrong state and can't be upgraded again, leaving all assets locked.

This is not the same as issues-55, it is not that the developer did not follow the best practice of 50-slots alignment, it is that the best practice was implemented with the wrong formula due to misunderstanding: The developer assume that struct always occupies 1 slot, a misconception that will likely lead to a future upgrade that will result in a completely broken contract that cannot be fixed with another upgrade.

Proof of Concept

According to the diff of 2.1.0-rc4...3.0.0, BasketHandlerP1@v3.0.0 added the following new storages:

    // === Function-local transitory vars ===

    // These are effectively local variables of _switchBasket.
    // Nothing should use their values from previous transactions.
    EnumerableSet.Bytes32Set private _targetNames;
    Basket private _newBasket;

    // === Warmup Period ===
    // Added in 3.0.0

    // Warmup Period
    uint48 public warmupPeriod; // {s} how long to wait until issuance/trading after regaining SOUND

    // basket status changes, mainly set when `trackStatus()` is called
    // used to enforce warmup period, after regaining SOUND
    uint48 private lastStatusTimestamp;
    CollateralStatus private lastStatus;

    // === Historical basket nonces ===
    // Added in 3.0.0

    // A history of baskets by basket nonce; includes current basket
    mapping(uint48 => Basket) private basketHistory;

    // Effectively local variable of `requireConstantConfigTargets()`
    EnumerableMap.Bytes32ToUintMap private _targetAmts; // targetName -> {target/BU}

And the __gap size is changed from 42 to 37 (means 5 slots are added):

-   uint256[42] private __gap;
+   uint256[37] private __gap;

Just like the CHANGELOG said:

BasketHandler [+5 slots] Summary: Introduces a notion of basket warmup to defend against short-term oracle manipulation attacks. Prevent RTokens from changing in value due to governance

However, the number of slots calculated in the documentation and the code is wrong, the real number of slots occupied by the new variables is 9 instead of 5:

2 slots for EnumerableSet.Bytes32Set private _targetNames;, because Bytes32Set is a struct as follows:

struct Bytes32Set {
Set _inner;
}
struct Set {
// Storage of set values
bytes32[] _values;
// Position of the value in the `values` array, plus 1 because index 0
// means a value is not in the set.
mapping(bytes32 => uint256) _indexes;
}

2 slots for: Basket private _newBasket;, because Basket is a struct as follows:

struct Basket {
IERC20[] erc20s; // enumerated keys for refAmts
mapping(IERC20 => uint192) refAmts; // {ref/BU}
}

1 slot for :

uint48 public warmupPeriod; // {s} how long to wait until issuance/trading after regaining SOUND
uint48 private lastStatusTimestamp;
CollateralStatus private lastStatus;

1 slot for: mapping(uint48 => Basket) private basketHistory;

3 slot for: EnumerableMap.Bytes32ToUintMap private _targetAmts;, because Bytes32ToUintMap is a struct as follows:

struct Bytes32ToUintMap {
Bytes32ToBytes32Map _inner;
}
struct Bytes32ToBytes32Map {
// Storage of keys
EnumerableSet.Bytes32Set _keys;
mapping(bytes32 => bytes32) _values;
}