cats - Protocol funds can be drained by auctioned NFT's owner using 2 wallets

[sherlock-admin3]

cats

high

Protocol funds can be drained by auctioned NFT's owner using 2 wallets

Summary

Protocol funds can be drained by auctioned NFT's owner by transferring it between 2 addresses.

Vulnerability Detail

If an owner of an auctioned NFT wants to bid on the auction, they only need to put down enough collateral to cover the fees for the bid amount they are making:

    if (bidder == currentBidder) {
        // If current bidder, collateral is entire fee amount
        feeAmount = totalCollateralAmount;

    require(
        _checkBidAmount(bidAmount, feeAmount),
        'EnglishPeriodicAuction: Incorrect bid amount'
    );

The issue is that the NFT owner can use a secondary address to leverage this in the following way (let's assume fee is 10% for simplicity):

1. Alice is the owner of the auctioned NFT, she makes the first bidAmount of 100 (sends 10 collateral for fee)
2. Alice transfers the NFT to Charlie
3. Charlie bids with bidAmount of 200 (sends only 20 collateral for fee since this is now the new owner addr)
4. Charlie transfers the NFT back to Alice
5. Auction ends, Charlie wins with highest bid 200
6. Charlie is different from oldBidder (current owner of token) and the highest bidder, so Charlie's winning bidAmount of 200 is transferred to Alice's collateral balance in _closeAuction()
7. NFT is transferred to Charlie

In that scenario, the total collateral put down by Alice using both her addresses is 30 collateral, but she receives (200 - collateralForFees), and also retains the NFT. In the end, Alice profits off of the protocol's balance. Her profit is (bidAmount - totalCollateralForFees). The bids can be repeatedly raised higher and higher using 2 wallets until enough bidAmount is accumulated to drain the contract's funds.

Impact

Protocol & user funds will be drained.

Code Snippet

https://github.com/sherlock-audit/2024-02-radicalxchange/blob/459dfbfa73f74ed3422e894f6ff5fe2bbed146dd/pco-art/contracts/auction/EnglishPeriodicAuctionInternal.sol#L500-L501

Tool used

Manual Review

Recommendation

Bidding logic is just flawed for auctions, deeper refactoring of the code is needed.

Duplicate of #2

[sherlock-admin2]

Escalate This is a valid issue and has absolutely nothing to do with #2, they are not duplicates.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[zzykxx]

This is invalid, NFTs cannot be transferred while they are being auctioned as described in this report.

[0xcats]

This is invalid, NFTs cannot be transferred while they are being auctioned as described in this report.

Agree, tried to remove escalation but could not in time

[Czar102]

Planning to reject the escalation and leave the issue as is.

[Czar102]

Result: Invalid Duplicate of #2

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 0xcats: rejected