search

code-423n4 / 2024-07-reserve-findings

5 stars 4 forks source link

Deprecated versions can still be used for RToken upgrades if Main has been upgraded #30

Closed c4-bot-2 closed 2 months ago

c4-bot-2 commented 3 months ago

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/Main.sol#L101 https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/Main.sol#L111

Vulnerability details

The MainP1 contract contains two upgrade functions: upgradeMainTo() and upgradeRTokenTo(). These functions are designed to upgrade the Main contract and the RToken components respectively. Only implementations that have been registered with the VersionRegistry and are not deprecated can be upgraded to.

upgradeMainTo() checks that the target version is not deprecated before upgrading as follows:

require(!versionRegistry.isDeprecated(versionHash), "version deprecated");

upgradeRTokenTo(), however, does not include this check. It only verifies that the Main contract has been upgraded to the given version:

require(keccak256(abi.encodePacked(this.version())) == versionHash, "upgrade main first");

If veRSR governance or the emergency council deprecate an implementation due to a security issue, it shouldn't be possible to finalize the upgrade even if the Main implementation had already been upgraded.

Impact

A version that may have been deprecated due to security concerns can still be upgraded to if the upgrade was already in progress.

Proof of Concept

  1. A new version (v2) is released and added to the VersionRegistry.
  2. An RToken's governance calls upgradeMainTo() to upgrade their Main contract to v2.
  3. A critical vulnerability is discovered in v2's RToken implementation.
  4. The emergency council deprecates v2 in the VersionRegistry.
  5. Despite the deprecation, governance can still call upgradeRTokenTo() with v2's versionHash, successfully upgrading the RToken components to the vulnerable version.

Tools Used

Manual review

Recommended Mitigation Steps

Add a deprecation check in the upgradeRTokenTo() function, similar to the one in upgradeMainTo(). This ensures that deprecated versions cannot be used for RToken upgrades and Main must be upgraded to a secure version first.

Assessed type

Upgradable

akshatmittal commented 3 months ago

This is intentional. If Main was upgraded, we want the entire protocol to be on the same version. Upgrades for RTokens will be atomic in nature, so Main and other parts will be upgraded at once. However you can see why Main needs to be upgraded first if the version requires additional upgrade related changes in Main those need to go in first before upgrading other parts of the protocol.

The version registry only prevents new RTokens from upgrading to deprecated versions but not disallow absolute usage.

c4-judge commented 2 months ago

thereksfour marked the issue as unsatisfactory: Invalid