Bid Beasts
First Flight #49
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Impact: high
Likelihood: high
Invalid

`BidBeastsNFTMarket:withdrawFee` Function, Incorrect Logic Leading to Possible Loss of All Fees by Admin

minos

BidBeastsNFTMarket:withdrawFee Function, Incorrect Logic Leading to Possible Loss of All Fees by Admin

Description

  • Under normal circumstances, the platform administrator should be able to withdraw the fees paid by users after transactions through the withdrawFee function.

  • However, due to the flawed logic in withdrawFee, if the administrator's actual address cannot receive Ether transfers (i.e., it is a contract account and does not have a receive or fallback function), calling this function will result in the loss of all fees.

function withdrawFee() external onlyOwner {
uint256 feeToWithdraw = s_totalFee;
require(feeToWithdraw > 0, "No fees to withdraw");
s_totalFee = 0;
_payout(owner(), feeToWithdraw);
emit FeeWithdrawn(feeToWithdraw);
}

Risk

Likelihood:

  • Only the administrator can call the withdrawFee function.

  • However, once the administrator's actual address cannot receive Ether transfers, disastrous consequences will occur.

Impact:

  • The administrator loses all fees.

Proof of Concept

  • Add the following code in BidBeastsMarketPlaceTest.t.sol:

function test__withdrawFee() public {
// Create seller account
address seller = makeAddr("seller");
vm.deal(seller, 10 ether);
// Mint NFT for seller
vm.prank(OWNER);
uint256 tokenId = nft.mint(seller);
// Seller lists NFT
vm.startPrank(seller);
nft.approve(address(market), tokenId);
market.listNFT(tokenId, MIN_PRICE, BUY_NOW_PRICE);
vm.stopPrank();
// Create buyer account
address buyer = makeAddr("buyer");
vm.deal(buyer, 10 ether);
// Buyer places a bid
vm.prank(buyer);
market.placeBid{value: MIN_PRICE + 1}(tokenId);
// Seller accepts the highest bid
vm.prank(seller);
market.takeHighestBid(tokenId);
console.log("255 address(market.owner()).balance = ", address(market.owner()).balance);
uint256 ownerBalance_Before = address(market.owner()).balance;
// Administrator withdraws fees
vm.prank(market.owner());
market.withdrawFee();
uint256 ownerBalance_After = address(market.owner()).balance;
vm.assertTrue(ownerBalance_Before == ownerBalance_After);
console.log("261 address(market.owner()).balance = ", address(market.owner()).balance);
}

Recommended Mitigation

- function withdrawFee() external onlyOwner {
+ function withdrawFee(address _receiver) external onlyOwner {
uint256 feeToWithdraw = s_totalFee;
require(feeToWithdraw > 0, "No fees to withdraw");
+ require(_receiver != address(0), "can't withdraw to zero address");
s_totalFee = 0;
- _payout(owner(), feeToWithdraw);
+ (bool success, ) = payable(_receiver).call{value: feeToWithdraw}("");
+ require(success, "withdrawFee Failed");
emit FeeWithdrawn(feeToWithdraw);
}
Updates

Lead Judging Commences

cryptoghost Lead Judge 2 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!