Token can be transferred during an auction, which would make the closeAuction() function revert and lock the highest bidders collateral.

Summary

A malicious user can transfer his token before an auction ends and make the closeAuction() function revert. This would lock the highest bidder's collateral in the protocol. This is caused by the StewardLicence contracts inheriting external ERC721 functions.

Vulnerability Detail

At the end of an auction the bid from the highest bidder is transferred to the previous StewardLicence owner. This is done via a call to the triggerTransfer() function of the StewardLicence contract:

https://github.com/sherlock-audit/2024-02-radicalxchange/blob/main/pco-art/contracts/auction/EnglishPeriodicAuctionInternal.sol#L526

    /**
     * @notice Close auction and trigger a transfer to the highest bidder
     */
    function _closeAuction(uint256 tokenId) internal {
        // (PART OF FUNCTION REMOVED FOR READABILITY)

        // Transfer to highest bidder
        IStewardLicense(address(this)).triggerTransfer(
            oldBidder,
            l.highestBids[tokenId][currentAuctionRound].bidder,
            tokenId
        );
        .
        .
    }

The StewardLicence is supposed to function as a manager contract for the ERC721s that are being auctioned periodically. Whenever an auction ends, the StewardLicence "tranfers" the ERC721 to it's new owner(licence holder), who holds it for the duration of the licence period, until the next auction starts. However, the contracts the compose the StewardLicence, specifically StewardLicenceInternal.sol inherit directly from ERC721Base from solidstate which exposes a number of ERC721 external/public functions like transfer and transferFrom.

Furthermore, the triggerTransfer() function, only routes the transferring to it's underlying ERC721 implementation, which is the one from solidstate. So it just sets the token owner in the ERC721Base to the auction winner. This means anyone can transfer their "temporarily licensed" token out and break the protocol.

This will break a lot of functions, but most obviously when someone wins an auction, they will not be able to close the auction and receive their new NFT. Furthermore, as the highest bidder cant take his collateral out due to design, his funds will be locked in the protocol(he can through cancelAllBids but this is a separate vulnerability that should be fixed).

Impact

Locked funds. Core protocol functionality is broken.

Code Snippet

StewardLicenceInternal, which inherits ERC721Base:

https://github.com/sherlock-audit/2024-02-radicalxchange/blob/main/pco-art/contracts/license/StewardLicenseInternal.sol#L20C1-L26C2

abstract contract StewardLicenseInternal is
    ERC721Base,
    ERC721Enumerable,
    ERC721Metadata,
    ERC165Base,
    AccessControlInternal
{
.
.
.

Tool used

Manual Review

Recommendation

StewardLicenceInternal should inherit ERC721BaseInternal, not ERC721Base.

sherlock-audit / 2024-02-radicalxchange-judging

gesha17 - Token can be transferred during an auction, which would make the closeAuction() function revert and lock the highest bidders collateral. #47