Skip to content

Latest commit

 

History

History
185 lines (132 loc) · 5.67 KB

014.md

File metadata and controls

185 lines (132 loc) · 5.67 KB

Steep Velvet Guppy

High

Incorrect Initialization will make OnlyOwner functions inaccessible ,owner will not receive funds meant for him and the contracts will remain in a paused state.

Summary

When initializing the OwnableUpgradeable library from openzeppelin, you normally use the __Ownable_init(_owner); , with your owner address, this will successfully set the right address for the contract.

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/access/OwnableUpgradeable.sol#L51C1-L53C6

    function __Ownable_init(address initialOwner) internal onlyInitializing {
        __Ownable_init_unchained(initialOwner);
    }

However this does not happen, as the both AuctionsHOuse contracts NounsAuctionHouseV2.sol and NounsAuctionHouseV3.sol have the initializer set __Ownable_init(); instead of __Ownable_init(_owner);

NounsAuctionHouseV2.sol

     * @notice Initialize the auction house and base contracts,
     * populate configuration values, and pause the contract.
     * @dev This function can only be called once.
     */
    function initialize(
        uint192 _reservePrice,
        uint56 _timeBuffer,
        uint8 _minBidIncrementPercentage
    ) external initializer {
        __Pausable_init();
        __ReentrancyGuard_init();
->        __Ownable_init();

        _pause();

        reservePrice = _reservePrice;
        timeBuffer = _timeBuffer;
        minBidIncrementPercentage = _minBidIncrementPercentage;

NounsAuctionHouseV3.sol

  /**
     * @notice Initialize the auction house and base contracts,
     * populate configuration values, and pause the contract.
     * @dev This function can only be called once.
     */
    function initialize(
        uint192 _reservePrice,
        uint56 _timeBuffer,
        uint8 _minBidIncrementPercentage,
        uint16 _immediateTreasuryBPs,
        uint16 _streamLengthInTicks,
        address _streamEscrow
    ) external initializer {
        __Pausable_init();
        __ReentrancyGuard_init();
->        __Ownable_init();

        _pause();

        reservePrice = _reservePrice;
        timeBuffer = _timeBuffer;
        minBidIncrementPercentage = _minBidIncrementPercentage;
        immediateTreasuryBPs = _immediateTreasuryBPs;
        streamLengthInTicks = _streamLengthInTicks;
        streamEscrow = IStreamEscrow(_streamEscrow);
    }

Both contracts also call the pause function in the initializer, which would mean that the paused state of the contract could not be reversed back.

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

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

When an aunction gets settled he owner is meant to receive ETH/WETH from _settleAuction inside NounsAuctionsHouseV2.sol and ``NounsAuctionHouseV3.sol` which will not send to the owner as the owner is never set!

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

        if (amountToSendTreasury > 0) {
            _safeTransferETHWithFallback(owner(), amountToSendTreasury);
        }
       
        
        if (_auction.amount > 0) {
            _safeTransferETHWithFallback(owner(), _auction.amount);
        }

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

Root Cause

In both bothNounsAuctionHouseV2.sol and NounsAuctionHouseV3.sol the ownable_init() is missing the owner's address inside the initializer function __ownable_init(owner) .

If you do not provide the address this will cause the owner never to be set for the OwnableUpgradeable.sol.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

  • All the onlyOwner cannot be called by the owner as the owner is never set.
  • The contract will remain in a paused state ,as in the initializer _pause is called and there is no owner to unpause it.
  • Owner will never receive the funds meant for the Owner.

PoC

No response

Mitigation

// NounsAuctionHouseV2.sol

    function initialize(
        uint192 _reservePrice,
        uint56 _timeBuffer,
        uint8 _minBidIncrementPercentage
+     address _owner
    ) external initializer {
        __Pausable_init();
        __ReentrancyGuard_init();
-      __Ownable_init();
+     __Ownable_init(_owner);

        _pause();

        reservePrice = _reservePrice;
        timeBuffer = _timeBuffer;
        minBidIncrementPercentage = _minBidIncrementPercentage;
    }

NounsAuctionHouseV3.sol

    function initialize(
        uint192 _reservePrice,
        uint56 _timeBuffer,
        uint8 _minBidIncrementPercentage,
        uint16 _immediateTreasuryBPs,
        uint16 _streamLengthInTicks,
        address _streamEscrow
+     address _owner 
    ) external initializer {
        __Pausable_init();
        __ReentrancyGuard_init();
-      __Ownable_init();
+     __Ownable_init(_owner);


        _pause();

        reservePrice = _reservePrice;
        timeBuffer = _timeBuffer;
        minBidIncrementPercentage = _minBidIncrementPercentage;
        immediateTreasuryBPs = _immediateTreasuryBPs;
        streamLengthInTicks = _streamLengthInTicks;
        streamEscrow = IStreamEscrow(_streamEscrow);
    }