Revolution Protocol - Ocean_Sky's results

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L348-L352

Vulnerability details

Impact

The _settleAuction function checks if the contract's balance is less than the reservePrice to determine the auction's outcome. If the reservePrice is changed right before settlement, it could unfairly alter the auction's result. The outcome will be the refund of eth or weth to the winner bidder and the burning of Verb nft.

function _settleAuction() internal {
        IAuctionHouse.Auction memory _auction = auction;

        require(_auction.startTime != 0, "Auction hasn't begun");
        require(!_auction.settled, "Auction has already been settled");
        //slither-disable-next-line timestamp
        require(block.timestamp >= _auction.endTime, "Auction hasn't completed");

        auction.settled = true;

        uint256 creatorTokensEmitted = 0;
        // Check if contract balance is greater than reserve price
        if (address(this).balance < reservePrice) { // if contract eth bal is more than reserve price, you can't execute this or burn the verb ID
            // If contract balance is less than reserve price, refund to the last bidder
            if (_auction.bidder != address(0)) {
                _safeTransferETHWithFallback(_auction.bidder, _auction.amount);
            }

            // And then burn the Noun
            verbs.burn(_auction.verbId);

This is basically resetting the whole process from voting up to the distribution of bid amount and verb nft to proper accounts, just because of untimely change of reserve price. Bidder participants might feel the auction rule is unpredictable and biased.

Proof of Concept

Please run this test to prove that reserve price can be change anytime in which it can alter the result of auction. Before you run this test, please follow this gist instruction https://gist.github.com/bluenights004/3a63b2d7916c951c9241d41c62a27b42

Test Detail:

function testSettlingAuctionReservePriceChange() public {
        createDefaultArtPiece();
        auction.unpause();

        uint256 bidAmount = auction.reservePrice(); // bidAmount is equal to reserve price of 1 ether
        address auctionOwner = auction.owner(); // auction owner address
         vm.deal(address(11), bidAmount); // setting up bidder's address and eth balance
        vm.startPrank(address(11));
        auction.createBid{ value: bidAmount }(0, address(11)); // Assuming first auction's verbId is 0
        vm.stopPrank();

        vm.warp(block.timestamp + auction.duration() + 1); // Fast forward time to end the auction

        vm.startPrank(auctionOwner);
        auction.setReservePrice(2 ether); // auction owner change the reserve price before settle auction into 2 ether
        auction.pause();
        vm.stopPrank();

        console.log("Eth balance of Last Bidder (before settle):", address(11).balance);
        console.log("Eth balance of Auction contract (before settle):", address(auction).balance);
        
    
        auction.settleAuction(); // settle the auction

        console.log("Eth balance of Last Bidder (after settle):", address(11).balance); // Ether is refunded to the bidder
        console.log("Eth balance of Auction contract (after settle):", address(auction).balance);     
    }

Result: Ether is refunded to the winner bidder

[PASS] testSettlingAuctionReservePriceChange() (gas: 983912)
Logs:
  Eth balance of Last Bidder (before settle): 0
  Eth balance of Auction contract (before settle): 1000000000000000000
  Eth balance of Last Bidder (after settle): 1000000000000000000
  Eth balance of Auction contract (after settle): 0

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 11.35ms

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Changing reserve price should not be done in critical period such as before the auction settlement. Consider limiting the time of its execution in which it can't affect the auction result.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/VerbsToken.sol#L193-L194

Vulnerability details

Impact

The function will return a URI for a token that doesn't actually exist, leading to confusion or misleading information.

function tokenURI detail:

 /**
     * @notice A distinct Uniform Resource Identifier (URI) for a given asset.
     * @dev See {IERC721Metadata-tokenURI}.
     */
    function tokenURI(uint256 tokenId) public view override returns (string memory) {
        return descriptor.tokenURI(tokenId, artPieces[tokenId].metadata);
    }

Based on EIP-721, the function tokenURI should throw an error if the provided tokenID is not valid nft minted by the contract.

Here's the link https://eips.ethereum.org/EIPS/eip-721

Past finding with similar issue: https://github.com/code-423n4/2023-04-caviar-findings/issues/44

Proof of Concept

Please run this test to prove that function tokenURI doesn't revert even if the tokenID is not minted. Before you run this test, please follow this gist instruction https://gist.github.com/bluenights004/0e1d63f0042582fd3f4314b819a826e0

Test Detail:

function testTokenURI() public {
        vm.stopPrank();
        vm.startPrank(address(auction));
        uint256 artPieceId = createDefaultArtPiece();
        uint256 tokenId = erc721Token.mint();
        (, ICultureIndex.ArtPieceMetadata memory metadata, , , , , , ) = cultureIndex.pieces(artPieceId);
        // Assuming the descriptor returns a fixed URI for the given tokenId
        string memory expectedTokenURI = descriptor.tokenURI(tokenId, metadata);
        assertEq(erc721Token.tokenURI(tokenId), expectedTokenURI, "Token URI should be correctly set and retrieved");
        
        // Test for a not minted token
    uint256 notMintedTokenId = tokenId + 1; // Assuming this tokenId has not been minted yet
    try erc721Token.tokenURI(notMintedTokenId) {
        // If this line is reached, it means the function did not revert as expected
        fail("tokenURI did not revert for a not minted tokenId");
    } catch (bytes memory) {
        // Catch the revert. This is the expected behavior for a not minted tokenId.
    }
    console.log("TokenURI of not minted TokenID:", erc721Token.tokenURI(notMintedTokenId));// shows the return data of TokenURI of invalid NFT
    } 

Result: The result shows here a failing test which means the tokenURI did not revert for a not minted tokenID

Running 1 test for test/verbs-token/Minting.t.sol:TokenMintingTest
[FAIL. Reason: assertion failed] testTokenURI() (gas: 994989)
Logs:
  Error: tokenURI did not revert for a not minted tokenId
  TokenURI of not minted TokenID: data:application/json;base64,eyJuYW1lIjoiVnJiIDEiLCAiZGVzY3JpcHRpb24iOiIuICIsICJpbWFnZSI6ICIiLCAiYW5pbWF0aW9uX3VybCI6ICIifQ==

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 11.15ms

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Follow EIP-721 by modifying the function in which it throws error message of "_tokenId` is not a valid NFT" if the provided tokenID is not minted by the contract.

Assessed type

ERC721