Skip to content

Latest commit

 

History

History
72 lines (49 loc) · 3.78 KB

027.md

File metadata and controls

72 lines (49 loc) · 3.78 KB

Scruffy Chartreuse Wolverine

Medium

Unprotected Upgradeable Contract Introduces Storage Collisions

Summary

The introduction of new state variables in NounsAuctionHouseV3 contract without proper storage layout management will cause storage slot collisions, leading to corrupted state data for the auction house, as upgrader improperly extends the contract without safeguarding existing storage slots.

Root Cause

In NounsAuctionHouseV3.sol, new state variables are added directly without ensuring proper storage alignment with NounsAuctionHouseV2.sol.

https://github.com/sherlock-audit/2024-11-nounsdao/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/NounsAuctionHouseV3.sol#L40

// In NounsAuctionHouseV3.sol
uint16 public immediateTreasuryBPs;
uint16 public streamLengthInTicks;
IStreamEscrow public streamEscrow;

Since NounsAuctionHouseV3 does not inherit from NounsAuctionHouseV2 but from the same base contracts, the newly introduced variables may overwrite storage slots used by NounsAuctionHouseV2, leading to storage slot collisions.

Internal Pre-conditions

  • The contract uses a proxy pattern for upgrades.
  • NounsAuctionHouseV3 inherits from the same base contracts as NounsAuctionHouseV2 but does not extend NounsAuctionHouseV2 directly.
  • New state variables are added in NounsAuctionHouseV3 without adjusting the storage layout.

External Pre-conditions

  • The DAO or an authorized upgrader initiates the upgrade from NounsAuctionHouseV2 to NounsAuctionHouseV3 without ensuring storage layout consistency.
  • NounsAuctionHouseV2 does not include a storage gap to accommodate future variables.
  • Comprehensive storage layout audits are not performed before deploying NounsAuctionHouseV3.

Attack Path

  • The DAO initiates the upgrade process, replacing NounsAuctionHouseV2 with NounsAuctionHouseV3 via the proxy admin.
  • Upon deployment, the new variables in NounsAuctionHouseV3 (immediateTreasuryBPs, streamLengthInTicks, streamEscrow) are assigned to storage slots already occupied by variables from NounsAuctionHouseV2.
  • Critical variables from V2, such as reservePrice, timeBuffer, or minBidIncrementPercentage, are overwritten by the new variables in V3.
  • An attacker manipulates the corrupted state variables. For example, setting reservePrice to an extremely low value allows placing unauthorized low bids.
  • Alternatively, manipulating streamEscrow redirects funds to unintended recipients.

Impact Assessment

Unauthorized manipulation of critical variables can lead to fund mismanagement, draining of reserves, and unauthorized transfers. Corrupted state variables can halt auction processes, prevent legitimate bids, or cause unexpected contract behavior.

## Mitigation Recommendations

Introduce reserved storage slots in NounsAuctionHouseV2 to accommodate future variables without affecting existing storage.

// In NounsAuctionHouseV2.sol
uint256[50] private __gap;

Ensure that NounsAuctionHouseV3 inherits directly from NounsAuctionHouseV2 to maintain a continuous storage layout.

// NounsAuctionHouseV3.sol
contract NounsAuctionHouseV3 is NounsAuctionHouseV2, INounsAuctionHouseV3 {
    // New variables
    uint16 public immediateTreasuryBPs;
    uint16 public streamLengthInTicks;
    IStreamEscrow public streamEscrow;
    
    // Storage gap for future upgrades
    uint256[45] private __gapV3;
}