022.md

Perfect Yellow Fox

Medium

Missing disableinitializers() in constructor can lead to malicious takeover of the implementation contracts

Summary disableinitializers is not called in NounsAuctionHouseV2's and NounsAuctionHouseV3's constructors.

Vulnerability Details An uninitialized contract can be taken over by an attacker. This applies to both a proxy and its implementation contract, which may impact the proxy. But in case of the implementation contract, a disableinitializers() is necessary to be called in the constructor. This is because when theNounsAuctionHouseV2 and NounsAuctionHouseV3 contract is deployed and initialized, the initialize method on the newly created NounsAuctionHouseV2 and NounsAuctionHouseV3 proxy's implementation contract is never called. As such, anyone can call that method and pass in whatever values they want as arguments. If an attacker passes in a contract as argument that calls selfdestruct, it will be run in the context of the NounsActionHouse implementation contract and will erase all code from that address.

Impact An attacker can call NounsAuctionHouseV2::initialize and NounsAuctionHouseV3::initialize and pass arbitrary arguments

Recommended Mitigation To prevent the implementation contract from being used, you should invoke the _disableInitializers() function from Openzeppelin in the constructor to automatically lock it when it is deployed:

• In NounsAuctionHouseV2 contract:

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

• In NounsAuctionHouseV3 contract:

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

reference Openzeppelin Docs