093.md

Amusing Currant Lynx

Medium

Malicious approved address for Noun token can permanently block auction

Summary

The StreamEscrow.createStream() allowing an approved address by protocol to create a stream with a nounId, provided the address is set in allowedToCreateStream mapping. But this is too risky because the address can permanently block all auctions in NounsAuctionHouseV3.

Root Cause

The root cause of the issue is the StreamEscrow contract is allowing an approved address to create a stream. This creates a centralization risk.

Internal pre-conditions

The attacked auction must have some ether to stream at the time of settlement so that StreamEscrow.forwardAllAndCreateaStream() is called. And the malicious address is set in allowedToCreateStream mapping.

External pre-conditions

One of approved address [ for Noun ] must be malicious.

Attack Path

Assume the minter is Noun auction proxy, NounId is 1 & protocol approved Alice to transfer the NounId. But here Alice got far more power than just transferring a token & creating a stream. She can permanently block all auctions. Lets see how:

1. The owner called NounsAuctionHouseV3.unpause(), a new auction was created for nounId 1, this noun was minted to Nouns auction proxy [ the proxy of NounsAuctionHouseV3 ] i.e the minter, so now the proxy is the owner of the noun of id 1.
2. Now after few bidding the auction came to the end & settleAuction() was called. Suppose this auction has 100 ether to stream.
3. Alice saw the settleAuction call and front-run it by calling createStream() with the nounId 1. In this call Alice passed 10000 wei as msg.value & 10000 as streamLengthInTick. As Alice is approved & allowed to create a stream the tx runs successfully. Now the escrow contract has a running stream for nounId 1.
4. Now settleAuction() executed, as there is 100 ether to stream StreamEscrow.forwardAllAndCreateStream() was called. When createStream() was called with nounId the tx reverts because of this check: require(!isStreamActive(nounId), 'stream active');. The createStream() call requires an non-active stream of the given nounId but we already have a stream of that nounId.

Now the problem is that we can't start a new auction if current auction is not settled. See this check from unpause() of NounsAuctionHouseV3 contract:

if (auctionStorage.startTime == 0 || auctionStorage.settled) {
            // @note if current auction is ongoing then it is not possible to create another auction.
            _createAuction();
        }

If settleAuction() is not executed successfully the state of the current auction will not be set to settled. So new auction can't be created.

Impact

New auction can't be created.

PoC

1. https://github.com/sherlock-audit/2024-11-nounsdao/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/StreamEscrow.sol#L114
2. https://github.com/sherlock-audit/2024-11-nounsdao/blob/main/nouns-monorepo/packages/nouns-contracts/contracts/NounsAuctionHouseV3.sol#L218-L220

Mitigation

As mentioned here the motive of StreamEscrow contract is receiving funds from auction house & create stream with those funds. i.e the stream is intended to create only with the returned ether from auction house i.e it is not intended to create an independent stream, just by calling StreamEscrow.createStream(). So it is better to only bound the authority to call the createStream() to the owner of the nounId i.e the auction house proxy. So instead of isApprovedOrOwner check that if the caller is the owner of the nounId.

function createStream(uint256 nounId, uint16 streamLengthInTicks) public payable {
        require(allowedToCreateStream[msg.sender], 'not allowed');
-       require(isApprovedOrOwner(msg.sender, nounId), 'only noun owner or approved');
+       require(nounsToken.ownerOf(nounId) == msg.sender)
        require(!isStreamActive(nounId), 'stream active');
// rest of codes