Damola0x - There could be a malicious takeover of NounsAuctionHouseV2.sol and NounsAuctionHouseV3.sol due to missing disableinitializers() in constructor

[sherlock-admin2]

Damola0x

Medium

There could be a malicious takeover of NounsAuctionHouseV2.sol and NounsAuctionHouseV3.sol due to missing disableinitializers() in constructor

Summary

The absence of 'disableinitializers()' in the constructor of NounsAuctionHouseV2.sol and NounsAuctionHouseV3.sol makes the contracts to be initialized by anyone after it has been deployed, an attacker could call the initialize method and pass it whatever values as arguments.

Root Cause

This issue is caused due to _disableInitializers() function from Openzeppelin not being called in the constructor of NounsAuctionHouseV2.sol and NounsAuctionHouseV3.sol to automatically lock the contracts when they are deployed, allowing the Initialize() function of the contracts to be called by anyone. https://github.com/sherlock-audit/2024-11-nounsdao/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/NounsAuctionHouseV2.sol#L72 and https://github.com/sherlock-audit/2024-11-nounsdao/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/NounsAuctionHouseV3.sol#L82

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

1. The NounsAuctionHouseV3.sol contract is being deployed but not yet initialized.
2. An attacker calls the Initialize() function and pass whatever values they want as arguments.
3. The attacker passes a contract that calls self-destruct as one of the arguments.
4. The arguments will run in NounsAuctionHouseV3.sol and will erase all the code from the address.

Impact

The protocol suffers as the attacker who initializes the contract could pass whatever values they want as argument, the could pass a contract as argument that calls selfdestruct, it would be run in context of the contracts and erase all code from their addresses.

PoC

No response

Mitigation

The _disableInitializers() function from Openzeppelin should be called in the constructor of the contracts to automatically lock them when they are deployed, preventing the implementation from being used:

constructor() {
    _disableInitializers();
}