Core Contracts
Regnum Aurum Acquisition Corp
HardhatReal World AssetsNFT
77,280 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Use of Non-Upgradeable ReentrancyGuard in Upgradeable Contract (StabilityPool) Leads to Storage Corruption

tinnohofficial

Summary

The StabilityPool contract inherits from ReentrancyGuard instead of ReentrancyGuardUpgradeable while being an upgradeable contract. This causes a storage collision risk since ReentrancyGuard uses a storage variable without storage gaps, potentially corrupting the contract's state variables during upgrades.

Vulnerability Details

The StabilityPool contract is an upgradeable contract but inherits from the non-upgradeable ReentrancyGuard. The issue arises because:

  • ReentrancyGuard has a state variable _status at slot 0 and StabilityPool has its own state variables starting from slot 1 (instead of slot 0 which is usually the intended for the base upgreadable smart contract)

  • Due to the inheritance, ReentrancyGuard's _status occupies slot 0, which should be reserved for StabilityPool's first variable.
    When the contract is upgraded, if the new implementation changes the storage layout, it could lead to storage collisions

Looking at the storage layout:

  • Slot 0: ReentrancyGuard._status

  • Slot 1: rToken

  • Slot 2: deToken

  • Slot 3: raacToken

  • Slot 4: lendingPool

abstract contract ReentrancyGuard {
// ...
uint256 private _status; // slot 0
}
​
contract StabilityPool is IStabilityPool, Initializable, ReentrancyGuard, OwnableUpgradeable, PausableUpgradeable {
// ...
IRToken public rToken; // slot 1
IDEToken public deToken; // slot 2
// ... and so on ...

This means the _status variable from ReentrancyGuard is overlapping with what should be the first state variable of StabilityPool.

A minor issue (can be ignored) - the ReentrancyGuard constructor is not effective as it being called in the context of the implementation and not the actual proxy. (this is why when we query it in the test in the PoC below we get 0) - NO real impact here since the _nonReentrantBefore in the nonReentrant modifier set it to NOT_ENTERED (2) anyways. but still something to take note of as initially there will be inconsistency btn implementation and the proxy.

constructor() {
// not effective since it is run in the context of the implementation (and not proxy)
_status = NOT_ENTERED;
}

On the other hand ReentrancyGuardUpgradeable uses custom storage slots which is better for avoiding such issues and a perfect fit for upgreadeable contracts.

struct ReentrancyGuardStorage {
uint256 _status;
}
​
bytes32 private constant ReentrancyGuardStorageLocation = 0x9b779b17422d0df92223018b32b4d1fa46e071723d6817e2486d003becc55f00;

In conclusion the issue is that the StabilityPool inherited from ReentrancyGuard instead of ReentrancyGuardUpgradeable there fore causing issues.

PoC

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
​
import {Test, console} from "forge-std/Test.sol";
import {StabilityPool} from "contracts/core/pools/StabilityPool/StabilityPool.sol";
import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
contract MockERC20 is ERC20 {
constructor(string memory name) ERC20(name, name) {}
}
​
contract StorageCorruption is Test {
address private alice = makeAddr("alice");
address private bob = makeAddr("bob");
​
StabilityPool private stabilityPool;
StabilityPool private implementation;
ERC1967Proxy private proxy;
​
address private mockRToken = address(new MockERC20("RToken"));
address private mockDeToken = address(new MockERC20("DeToken"));
address private mockRaacToken = address(new MockERC20("RaacToken"));
address private mockCrvUSDToken = address(new MockERC20("crvToken"));
address private mockRaacMinter = makeAddr("raac_minter");
address private mockLendingPool = makeAddr("lending_pool");
​
function setUp() public {
// 1. Deploy implementation contract
implementation = new StabilityPool(address(this));
​
// 2. Encode initialization data
bytes memory initData = abi.encodeWithSelector(
StabilityPool.initialize.selector,
mockRToken,
mockDeToken,
mockRaacToken,
mockRaacMinter,
mockCrvUSDToken,
mockLendingPool
);
​
// 3. Deploy proxy pointing to implementation
proxy = new ERC1967Proxy(address(implementation), initData);
​
// 4. Create interface to interact with proxy
stabilityPool = StabilityPool(address(proxy));
}
​
function testSlots() public view {
bytes32 value = vm.load(address(stabilityPool), bytes32(0));
// @audit-info the slot 0 is occupied by the _status variable in ReentrancyGuard
assertEq(uint256(value), 0);
for (uint256 i = 1; i < 5; i++) {
value = vm.load(address(stabilityPool), bytes32(i));
if (i == 1) assertEq(address(uint160(uint256(value))), mockRToken);
else if (i == 2) assertEq(address(uint160(uint256(value))), mockDeToken);
else if (i == 3) assertEq(address(uint160(uint256(value))), mockRaacToken);
else if (i == 4) assertEq(address(uint160(uint256(value))), mockLendingPool);
}
}
}

Impact

Storage corruption when upgrading.

Tools Used

  • Manual Review

  • Foundry Test Framework

Recommendations

Replace ReentrancyGuard with ReentrancyGuardUpgradeable:

- import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
+ import "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
​
- contract StabilityPool is IStabilityPool, Initializable, ReentrancyGuard, OwnableUpgradeable, PausableUpgradeable {
+ contract StabilityPool is IStabilityPool, Initializable, ReentrancyGuardUpgradeable, OwnableUpgradeable, PausableUpgradeable {
​
function initialize(...) public initializer {
+ __ReentrancyGuard_init();
...
}
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Known issue

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!