search

code-423n4 / 2022-09-nouns-builder-findings

10 stars 6 forks source link

Possible for funds to get stuck in the Auction contract #668

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/main/src/auction/Auction.sol#L244-L260

Vulnerability details

Impact

For projects with a 0% founder percentage ownership, it is possible for the highest bidder of the first auction to not receive the minted token, and funds from the first auction to be stuck in the contract.

Proof of Concept

In Auction.sol, the contract determines if the first auction has been created using the statement auction.tokenId == 0, as shown below:

244:    function unpause() external onlyOwner {
245:        _unpause();
246:
247:        // If this is the first auction:
248:        if (auction.tokenId == 0) {
249:            // Transfer ownership of the contract to the DAO
250:            transferOwnership(settings.treasury);
251:
252:            // Start the first auction
253:            _createAuction();
254:        }
255:        // Else if the contract was paused and the previous auction was settled:
256:        else if (auction.settled) {
257:            // Start the next auction
258:            _createAuction();
259:        }
260:    }

If the check at line 248 passes, the unpause() function will directly call _createAuction() without checking that the current auction has been settled.

Thus, in a situation where the tokenId of the first auction is 0, the owner could potentially call unpause() again without settling the first auction, causing the current auction to be overwritten due to the call to _createAuction().

This would cause the following:

For example:

In this scenario, funds from the first auction would be unretrievable as the Auction contract does not have a function to send any amount of ETH to an address. Furthermore, as settleAuction() was not called, the highest bidder would not receive the token of the first auction.

The test in this gist demonstrates the above.

Recommended Mitigation Steps

At line 248, instead of checking auction.tokenId == 0 to determine if the first auction has been created, check if auction.startTime == 0 instead.

GalloDaSballo commented 2 years ago

Dup of #376