FassiSecurity - User can cause art piece to always go to the repossessor

[sherlock-admin2]

FassiSecurity

high

User can cause art piece to always go to the repossessor

Summary

If a user wants to withdraw his available collateral, he can do so by calling _withdrawCollateral.

    /**
     * @notice Withdraw collateral
     */
    function _withdrawCollateral(address bidder) internal {
        EnglishPeriodicAuctionStorage.Layout
            storage l = EnglishPeriodicAuctionStorage.layout();

        uint256 collateralAmount = l.availableCollateral[bidder];

        require(
            collateralAmount > 0,
            'EnglishPeriodicAuction: No collateral to withdraw'
        );

        // Make collateral unavailable to withdraw
        l.availableCollateral[bidder] = 0;

        // Transfer collateral back to bidder
        //slither-disable-next-line low-level-calls
        (bool success, ) = bidder.call{ value: collateralAmount }('');
        require(
            success,
            'EnglishPeriodicAuction: Failed to withdraw collateral'
        );
    }

Vulnerability Detail

After the auction ends _closeAuction can be called by anyone.

 /**
     * @notice Close auction and trigger a transfer to the highest bidder
     */
    function _closeAuction(uint256 tokenId) internal {
        EnglishPeriodicAuctionStorage.Layout
            storage l = EnglishPeriodicAuctionStorage.layout();

        uint256 currentAuctionRound = l.currentAuctionRound[tokenId];

        address oldBidder;
        if (IStewardLicense(address(this)).exists(tokenId)) {
            oldBidder = IStewardLicense(address(this)).ownerOf(tokenId);
        } else {
            oldBidder = l.initialBidder;
        }

        // Set lastPeriodEndTime to the end of the current auction period
        uint256 licensePeriod = IPeriodicPCOParamsReadable(address(this))
            .licensePeriod();

        l.lastPeriodEndTime[tokenId] = block.timestamp;
        l.currentLicensePeriod[tokenId] = licensePeriod;

        if (l.highestBids[tokenId][currentAuctionRound].bidder == address(0)) {
            // No bids were placed, transfer to repossessor
            Bid storage repossessorBid = l.bids[tokenId][currentAuctionRound][
                l.repossessor
            ];
            repossessorBid.bidAmount = 0;
            repossessorBid.feeAmount = 0;
            repossessorBid.collateralAmount = 0;
            repossessorBid.bidder = l.repossessor;

            l.highestBids[tokenId][currentAuctionRound] = repossessorBid;
        } else if (
            l.highestBids[tokenId][currentAuctionRound].bidder != oldBidder
        ) {
            // Transfer bid to previous bidder's collateral
            l.availableCollateral[oldBidder] += l
            .highestBids[tokenId][currentAuctionRound].bidAmount;
            l.highestBids[tokenId][currentAuctionRound].collateralAmount = 0;
            l
            .bids[tokenId][currentAuctionRound][
                l.highestBids[tokenId][currentAuctionRound].bidder
            ].collateralAmount = 0;
        } else {
            l.highestBids[tokenId][currentAuctionRound].collateralAmount = 0;
            l
            .bids[tokenId][currentAuctionRound][oldBidder].collateralAmount = 0;
        }

        emit AuctionClosed(
            tokenId,
            currentAuctionRound,
            l.highestBids[tokenId][currentAuctionRound].bidder,
            oldBidder,
            l.highestBids[tokenId][currentAuctionRound].bidAmount
        );

        // Reset auction
        l.currentAuctionLength[tokenId] = 0;
        l.currentAuctionRound[tokenId] = l.currentAuctionRound[tokenId] + 1;

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

        // Distribute fee to beneficiary
        if (l.highestBids[tokenId][currentAuctionRound].feeAmount > 0) {
            IBeneficiary(address(this)).distribute{
                value: l.highestBids[tokenId][currentAuctionRound].feeAmount
            }();
        }
    }

This will distribute the following as the docs state:

//- Transferring the associated NFT from the now-former Steward to the winning bidder (if applicable)
//- Releasing the winning Bid Value from the winning bidder's Locked Collateral to the former Steward's Available Collateral balance (if applicable)
//- Transferring the Honorarium from the winning bidder's Locked Collateral to the Creator Circle distribution mechanism

Impact

Now, because _withdrawCollateral does not have a check for the highest bidder, Bob can make sure every auction will end up going to the repossessor; this will be shown with an example:

• The auction starts, Bob bids an extremely high amount, ensuring he is the only one who placed a bid.
• Auction ends, Bob frontruns _closeAuction by calling _cancellAllBidsAndWithdrawCollateral
• Because _WithdrawCollateral does not have the aforementioned check implemented, Bob can now after cancelling all his bids, withdraw his collateral.
• Since Bob was the only bidder and now is removed, there will be no bidder left.
• The art piece will end up going to the repossessor.

  Code Snippet

  https://github.com/sherlock-audit/2024-02-radicalxchange/blob/459dfbfa73f74ed3422e894f6ff5fe2bbed146dd/pco-art/contracts/auction/facets/EnglishPeriodicAuctionFacet.sol#L185-L190

  Tool used

Manual Review

Recommendation

Make sure to implement a check that ensures the highest bidder cannot withdraw his collateral.

Duplicate of #14