Nouns Builder contest - pauliax's results

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L247-L254 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L206-L208 https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/token/Token.sol#L154

Vulnerability details

Impact

Token ids start from 0, however, this value is also checked against when unpausing the contract:

  function unpause() external onlyOwner {
    _unpause();

    // If this is the first auction:
    if (auction.tokenId == 0) {
        // Transfer ownership of the contract to the DAO
        transferOwnership(settings.treasury);

        // Start the first auction
        _createAuction();
    }
    ...
  }

If the owner unpauses the auction twice, the second time the value of auction.tokenId is again 0 meaning the check will pass and re-create the auction again even though the current auction might have active bids. It is highly likely because I think the highest chances of unpausing are initially if something does not go according to the plan or an owner wants to adjust some parameters before continuing the sale. This could happen accidentally and the current highest bid and NFT #0 will be lost or this could be a griefing attack by the malicious owner which I suppose is very unlikely because the owner has too many privileges to exploit the system anyway. Yet I think this is a significant flaw in the codebase that should be prevented and deserves a severity of Medium.

Proof of Concept

PoC of a simplified version: https://gist.github.com/pauliax/07ba76afb89a431f78eadef488e296e0

Steps to reproduce are in the gist's comments section.

Recommended Mitigation Steps

Token ids should either not start from 0 or you should enhance this check, e.g. if (auction.tokenId == 0 && auction.startTime == 0).

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L363-L364

Vulnerability details

Impact

This situation happens when the proposer's voting power and proposalThreshold are equal because the contract does not respect that:

  function cancel(bytes32 _proposalId) external {
    // Ensure the caller is the proposer or the proposer's voting weight has dropped below the proposal threshold
    if (msg.sender != proposal.proposer && getVotes(proposal.proposer, block.timestamp - 1) > proposal.proposalThreshold)
        revert INVALID_CANCEL();

The original Nouns contract had these checks in an opposite way:

  require(
      msg.sender == proposal.proposer ||
          nouns.getPriorVotes(proposal.proposer, block.number - 1) < proposal.proposalThreshold,
      'NounsDAO::cancel: proposer above threshold'
  );

so when inverting, < should become >=. Now when proposer votes == proposal.proposalThreshold it allows canceling such a proposal by anyone. This is a huge flow compared to ERC20s-based votes because here this 1 vote means 1 NFT which is supposed to be significant and hard to obtain. E.g. when the proposalThreshold is 2 NFTs the proposer must have 3 NFTs to avoid being canceled.

Recommended Mitigation Steps

getVotes(proposal.proposer, block.timestamp - 1) >= proposal.proposalThreshold

Judge has assessed an item in Issue #704 as Medium risk. The relevant finding follows:

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/governance/governor/Governor.sol#L111-L175

Vulnerability details

Impact

In the initial phase, when not many tokens are minted, a malicious actor can start submitting proposals and later execute them. E.g. when the first token is minted, this first owner can instantly submit proposals to retrieve all the eth back from the treasury. proposalThreshold and quorum will likely be low (0 or 1 depending on proposalThresholdBps and quorumThresholdBps).

Others would not have a chance to block this proposal later because their past votes were 0. The admins can jump in and veto such proposals but this does not prevent an attacker from spamming multiple proposals simultaneously making it infeasible to fight if the attacker is willing to spend enough ETH for gas.

A similar issue is when the total supply is 0. Then anyone can submit proposals because the threshold and quorum are basically 0. Admins will not have enough supplies to combat the aggregated power of all the spammers.

Thus, this initial spam of proposals can become insurmountable even with the veto function.

Recommended Mitigation Steps

I believe veto is not enough to combat this issue and there should be more on-chain precautions. Consider something like introducing a minimum token supply when the proposals can start or limiting the number of active proposals per token.

• I think it would be better if changes to essential auction parameters (setDuration, setReservePrice, setTimeBuffer, setMinimumBidIncrement, etc) did not impact the current auction. They should take effect only when the next auction round starts. The current ongoing auction should finish with the old settings so that users won't be front-runned with unexpected new settings that they were not bidding on. Or at least these settings should have reasonable upper/lower boundaries to prevent the possibility of griefing. This is submitted as a QA issue because it was explicitly mentioned: "Wardens should assume that governance variables are set sensibly".

• There are many constructors that are payable for no reason, especially when there is no way to retrieve the native asset later unless the contract is upgraded.

• Token in AuctionStorageV1 should probably be of an interface type, that is IToken, not implementation:

  Token public token;

• minBid can have a minimum value of 1%, consider increasing bps to allow broader spreads:

  minBid = highestBid + ((highestBid * settings.minBidIncrement) / 100);

• When executing the proposal, it does not check that msg.value == sum of all _values, the caller will lose excess eth, it will remain in the treasury:

  function execute(
        ...
    ) external payable returns (bytes32) {
        ...
        settings.treasury.execute{ value: msg.value }(_targets, _values, _calldatas, _descriptionHash);

• I am not sure if this is a feature or a bug but createBid does not have whenNotPaused modifier, meaning the bids for the current auction can still come even when the contract is paused:

  function createBid(uint256 _tokenId) external payable nonReentrant

On one hand, it is bad in case you discover a flaw and want to stop bidding, on the other hand, it is good that you let finish the current auction and the owners cannot interrupt it by pausing and unpausing when it is beneficial to them. So because I was not sure what was the original intention, I am submitting this as a QA issue.

Repeated call to proposalThreshold():

  // Get the current proposal threshold
  uint256 currentProposalThreshold = proposalThreshold();

  // Cannot realistically underflow and `getVotes` would revert
  unchecked {
    // Ensure the caller's voting weight is greater than or equal to the threshold
    if (getVotes(msg.sender, block.timestamp - 1) < proposalThreshold()) revert BELOW_PROPOSAL_THRESHOLD();
  }