Root + Impact

Description

The BidBeastsNFTMarketPlace.sol::withdrawAllFailedCredits() allows users with failed credits to withdraw them to a recipient address they prefer. However, the function incorrectly reads the credits of the receiver failedTransferCredits[_receiver], instead of the caller, which is wrong as receiver is not obligated to have failed credits - receiver just acts as a transfer destination. Also, the funds are never send to the receiver but to the caller. This flawed flow gives attackers a perfect opportunity to exploit it and steal funds.

Also, since the _receiver’s balance is never reduced, the same credits can be withdrawn multiple times, allowing an attacker to completely drain the contract’s ETH.

Risk

Likelihood:

High.

Impact:

• Users failed credits can be stolen by malicious actors.

• The market's ETH can be drained completely.

Proof of Concept

Consider the following example:

1. Admin mints an NFT to Alice.

2. Alice lists her NFT in the market.

3. The auction begins and Bob places his bid.

4. Another user comes and places a bigger bid - maybe wins the auction.

5. Bob will be refunded his bid, but if something goes wrong, the tx wil not revert, but instead the failed credits of Bob will be increased, allowing him to redeem them later to a new receiver address via withdrawAllFailedCredits.

6. An attacker can frontrun Bob and pass receiver = Bob.

7. Bob's credit will be send to attacker.

This is just one attack path, but the failed credits can be stolen from the nft seller as well, or any user who has a record in the failedTransferCredits mapping.

Recommended Mitigation

Instead of reading the amount to withdraw from the receiver record => amount = failedTransferCredits[_receiver], use msg.sender and in the .call transfer the funds to the receiver address.