code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/main/package.json#L32-L33 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/NounsAuctionHouse.sol#L62-L82

Vulnerability details

Impact

Currently, @openzeppelin/contracts and @openzeppelin/contracts-upgradeable versions are set as follows.

https://github.com/code-423n4/2022-08-nounsdao/blob/main/package.json#L32-L33

    "@openzeppelin/contracts": "^4.1.0",
    "@openzeppelin/contracts-upgradeable": "^4.1.0",

For the specified version, there are some critical and high vulnerabilities as shown in https://snyk.io/test/npm/@openzeppelin/contracts/4.1.0. One of these vulnerabilities deals with the initializer modifier in which https://security.snyk.io/vuln/SNYK-JS-OPENZEPPELINCONTRACTS-2320176 mentions that "it is possible for initializer() protected functions to be executed twice, if this happens in the same transaction".

The initializer modifier is currently used in the NounsAuctionHouse contract. Although NounsAuctionHouse is not in the scope of this contest, because of the nature of high vulnerability, this finding is issued so the protocol can be aware of this risk. Moreover, since the Openzeppelin contracts are used in multiple places, the protocol should review these vulnerabilities carefully so these risks can be avoided when using a feature from these contracts in the future.

Proof of Concept

Because of the @openzeppelin/contracts and @openzeppelin/contracts-upgradeable versions specified in package.json, Openzeppelin contracts of version, such as 4.1.0, can be installed.

In the NounsAuctionHouse contract, the initializer modifier is used for the following initialize function. Due to the high vulnerability related to initializer mentioned above, the initialize function can be executed twice to cause the state variables, such as weth, to be set to undesired and malicious values.

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/NounsAuctionHouse.sol#L62-L82

    function initialize(
        INounsToken _nouns,
        address _weth,
        uint256 _timeBuffer,
        uint256 _reservePrice,
        uint8 _minBidIncrementPercentage,
        uint256 _duration
    ) external initializer {
        __Pausable_init();
        __ReentrancyGuard_init();
        __Ownable_init();

        _pause();

        nouns = _nouns;
        weth = _weth;
        timeBuffer = _timeBuffer;
        reservePrice = _reservePrice;
        minBidIncrementPercentage = _minBidIncrementPercentage;
        duration = _duration;
    }

Tools Used

VSCode

Recommended Mitigation Steps

In package.json, set @openzeppelin/contracts and @openzeppelin/contracts-upgradeable versions to the latest. To fix the issue related to initializer, version 4.4.1 or higher should be used.

eladmallel commented 2 years ago

The risk is low for Nouns DAO since our Auction House is already deployed and initialized. However we do agree it's good to upgrade the open zeppelin library version and plan to do so.

gzeoneth commented 2 years ago

Not exploitable given the current state of the contract.

code-423n4 / 2022-08-nounsdao-findings