Revolution Protocol - leegh's results

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/tree/main/packages/revolution/src/AuctionHouse.sol#L273-L301

Vulnerability details

Impact

An auction's parameters (timeBuffer, reservePrice and minBidIncrementPercentage) can be changed anytime by contract owner, even when the auction is still in active. The changes will be effective immediately in createBid and _settleAuction, but users may not realize that. Possible impacts include:

1. reservePrice and minBidIncrementPercentage changes result bidding transactions fail.
2. reservePrice change results in unsuccessful auction if the new reservePrice is larger than current highest bid, and no more bids after that.
3. users get unexpected result, as they may change their bidding strategy if auction parameters change.

Proof of Concept

The set functions for timeBuffer, reservePrice and minBidIncrementPercentage are only limited to called by owner, and have no timing limit. Owner can change them anytime, even when an active auction exists. These changes take effect immediately in createBid and _settleAuction, which may create undefined behaviors, like fail of bidding, change of price.

    /**
     * @notice Set the auction time buffer.
     * @dev Only callable by the owner.
     */
    function setTimeBuffer(uint256 _timeBuffer) external override onlyOwner {
        timeBuffer = _timeBuffer;

        emit AuctionTimeBufferUpdated(_timeBuffer);
    }

    /**
     * @notice Set the auction reserve price.
     * @dev Only callable by the owner.
     */
    function setReservePrice(uint256 _reservePrice) external override onlyOwner {
        reservePrice = _reservePrice;

        emit AuctionReservePriceUpdated(_reservePrice);
    }

    /**
     * @notice Set the auction minimum bid increment percentage.
     * @dev Only callable by the owner.
     */
    function setMinBidIncrementPercentage(uint8 _minBidIncrementPercentage) external override onlyOwner {
        minBidIncrementPercentage = _minBidIncrementPercentage;

        emit AuctionMinBidIncrementPercentageUpdated(_minBidIncrementPercentage);
    }

AuctionHouse.sol#L273-L301

Tools Used

vscode

Recommended Mitigation Steps

Protect the set functions with modifier whenPaused, or cache the parameters of auction for an ongoing auction.

Assessed type

Other

Low

[L-01] validateMediaType does not check the metadata of AUDIO art piece.

The metadata of all art piece types are defined and validated except AUDIO type.

File: packages/revolution/src/CultureIndex.sol

159:    function validateMediaType(ArtPieceMetadata calldata metadata) internal pure {
160:        require(uint8(metadata.mediaType) > 0 && uint8(metadata.mediaType) <= 5, "Invalid media type");
161:
162:        if (metadata.mediaType == MediaType.IMAGE)
163:            require(bytes(metadata.image).length > 0, "Image URL must be provided");
164:        else if (metadata.mediaType == MediaType.ANIMATION)
165:            require(bytes(metadata.animationUrl).length > 0, "Animation URL must be provided");
166:        else if (metadata.mediaType == MediaType.TEXT)
167:            require(bytes(metadata.text).length > 0, "Text must be provided");  // @audit no checking for AUDIO type
168:    }

CultureIndex.sol#L159-L168

File: packages/revolution/src/interfaces/ICultureIndex.sol

88:    struct ArtPieceMetadata {
89:        string name;
90:        string description;
91:        MediaType mediaType;
92:        string image;
93:        string text;
94:        string animationUrl;  // @audit no metadata for AUDIO type
95:    }

ICultureIndex.sol#L88-L95

[L-02] Animation URL is mandatory, not optional.

The comment for metadata is incorrect (L205). The animation URL is mandatory and not optional according to validateMediaType(L164-L165).

File: packages/revolution/src/CultureIndex.sol

164:        else if (metadata.mediaType == MediaType.ANIMATION)
165:            require(bytes(metadata.animationUrl).length > 0, "Animation URL must be provided");

205: * - `metadata` must include name, description, and image. Animation URL is optional.  // @audit incorrect comment

CultureIndex.sol#L205

[L-03] Incorrect comment for newQuorumVotesBPS.

The hardcoded quorum votes bps (MAX_QUORUM_VOTES_BPS) is max instead of min.

File: packages/revolution/src/CultureIndex.sol

495:     * @dev newQuorumVotesBPS must be greater than the hardcoded min

CultureIndex.sol#L495

[L-04] Require the token is minted when querying metadata of the token.

First check if the token is minted before returning the token's metadata, avoiding return invalid data for non-existing token.

File: packages/revolution/src/VerbsToken.sol

193:    function tokenURI(uint256 tokenId) public view override returns (string memory) {
194:        return descriptor.tokenURI(tokenId, artPieces[tokenId].metadata);   // @audit `_requireOwned(tokenId)`
195:    }

200:    function dataURI(uint256 tokenId) public view override returns (string memory) {
201:        return descriptor.dataURI(tokenId, artPieces[tokenId].metadata);    // @audit `_requireOwned(tokenId)`
202:    }

VerbsToken.sol#L193-L195,VerbsToken.sol#L200-L202

[L-05] Consider adding unlock functions for minter, descriptor and cultureIndex.

The lock functions (lockMinter, lockDescriptor, lockCultureIndex) are risky when no unlock functions are provided. Once locked, there's no way to unlock it, which means it is unable to upgrade minter, descriptor and cultureIndex.

File: packages/revolution/src/VerbsToken.sol

220:    function lockMinter() external override onlyOwner whenMinterNotLocked
242:    function lockDescriptor() external override onlyOwner whenDescriptorNotLocked {
262：   function lockCultureIndex() external override onlyOwner whenCultureIndexNotLocked {

VerbsToken.sol#L220, VerbsToken.sol#L242, VerbsToken.sol#L262

[L-06] Incorrect checking for auction reservePrice.

Use _auction.amount < reservePrice instead of address(this).balance < reservePrice, as _auction.amount is the real bid amount of the auction and it is possible that address(this).balance != _auction.amount.

File: packages/revolution/src/AuctionHouse.sol

348:        if (address(this).balance < reservePrice) {  // @audit use `_auction.amount < reservePrice`
349:            // If contract balance is less than reserve price, refund to the last bidder
350:            if (_auction.bidder != address(0)) {
351:                _safeTransferETHWithFallback(_auction.bidder, _auction.amount);
352:            }

AuctionHouse.sol#L348-L352

Non-Critical

[N-01] Check creator's bps is larger than 0.

File: packages/revolution/src/CultureIndex.sol

185:        for (uint i; i < creatorArrayLength; i++) {
186:            require(creatorArray[i].creator != address(0), "Invalid creator address");
187:            totalBps += creatorArray[i].bps; // @audit require `creatorArray[i].bps != 0`
188:        }

CultureIndex.sol#L185-L188