OlaHamid - **Improper Implementation of Upgradable Contract Design in AuctionHouse Contract**

[sherlock-admin3]

OlaHamid

Medium

Improper Implementation of Upgradable Contract Design in AuctionHouse Contract

SUMMARY

The contract appears to be designed as an upgradable contract as it uses openZepplin upgradeable pattern but lacks critical implementation details, raising questions about its compliance with best practices for upgradable proxy patterns. Specifically, issues related to the initializer modifier, unprotected initialization logic, and improper deactivation of the initializer post-deployment are highlighted.

---

https://github.com/sherlock-audit/2024-11-nounsdao/blob/8b6fb94f103134e751cf016e5c3f4185be89bb49/nouns-monorepo/packages/nouns-contracts/contracts/NounsAuctionHouseV2.sol#L83

ROOT CAUSE

1. Incorrect Use of initializer:

   • The constructor is marked with the initializer modifier, but there is no mechanism to prevent reinitialization after deployment, leading to potential state corruption.
   • No initializing state check is implemented post-deployment to ensure immutability of the initialization process.

2. Lack of Proxy-Compatible Storage Layout Management:

   • The initialize function does not explicitly indicate compatibility with a proxy-based storage layout (e.g., storage gaps are missing).

3. Misalignment of Upgradable Contract Practices:

   • Key contract-level initializations (e.g., __Pausable_init(), __Ownable_init()) are correctly called but lack defensive programming to ensure proper initialization occurs only once. example if it decides to follow the UUPS patern the constructor should be called _disableInitializers() function in it's contrctor logic.

---

INTERNAL PRECONDITION

The constructor and initialize function can both be invoked during deployment due to improper safeguards, allowing for potential overwriting of critical state variables.

---

EXTERNAL PRECONDITION

An attacker or malicious actor with external access to the initialize function after deployment could:

• Reset state variables.
• Alter auction parameters such as reservePrice, timeBuffer, and minBidIncrementPercentage.

---

ATTACK PATH

1. Deploy the contract, calling the constructor.
2. Exploit the unprotected initialize function to overwrite sensitive data or initialize the contract multiple times.

---

IMPACT

• High: Contract functionality and state integrity are compromised. Initialization logic is a critical security concern, especially in upgradable contracts, as it determines the integrity of deployed contracts.
• Potential Exploits: Attackers could reset critical configurations, bypass access control mechanisms, or disrupt auction logic.

---

PROOF OF CONCEPT (PoC)

1. Deploy the contract using the provided constructor:

   constructor(INounsToken _nouns, address _weth, uint256 _duration) initializer {
       nouns = _nouns;
       weth = _weth;
       duration = _duration;
   }

2. Invoke the initialize function post-deployment with attacker-controlled parameters:

   initialize(1 ether, 120, 5);

3. Observe overwritten auction parameters:

   • reservePrice = 1 ether
   • timeBuffer = 120
   • minBidIncrementPercentage = 5

---

MITIGATION

1. Use the Correct Constructor-Initializer Approach for Upgradable Contracts:

   • Remove the initializer modifier from the constructor. Constructors should be empty in upgradable contracts.

2. Restrict Initialization Logic:

   • Ensure initialize is callable only once by the proxy. Use OpenZeppelin's Initializable library correctly, which includes _initialized and _initializing safeguards.

3. Deactivate Initializer Post-Deployment:

   • Include explicit checks and a mechanism to mark the initialization as complete:

     function initialize(...) external initializer {
         require(!_initialized, "Already initialized");
         _initialized = true;
         ...
     }

4. Audit Storage and Initialization Patterns:

   • Conduct a thorough review to ensure the contract adheres to the proxy storage and initialization rules.

OR

• strictly follow the UUPS INITIALIZATION PROXY PATTERN AND ensure that the _disableInitializers() is added to the constructor.