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

Missing checks in the `BidBeastsNFTMarket::withdrawAllFailedCredits` function

elo_anxiety

Missing checks in the BidBeastsNFTMarket::withdrawAllFailedCredits function ,making it extremely easy to grab someone else's fund which were not transfered successfully during the _payout internal function call .


Description: In the BidBeastsNFTMarket::withdrawAllFailedCredits function there is no check if msg.sender is the _reciever. Hence, if transaction of some user is failed by some reason in the BidBeastsNFTMarket::_payout function ,anyone can withdraw that money by calling the withdrawAllFailedCredits function by passing the victim's address and receiving the funds on the attacker's address.

function withdrawAllFailedCredits(address _receiver) external {
@> // there must be a check here which checks if `_receiver`==`msg.sender`.
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");
}
​

Likelihood Medium

The (bool success, ) = payable(recipient).call{...} will return false primarily if the recipient is a smart contract that:

  1. Does not have a receive() or a payable fallback() function to accept the Ether.

  2. Has a function to receive Ether, but that function uses more than 2300 gas (the amount forwarded by .call by default).

  3. The receiving function intentionally reverts.


Impact: High

  • If some user places a bid ,but a higher bid is placed by some other user ,then at the time of reallocation of funds to the previous user , if that transaction is failed by any reason , anyone can withdraw the funds of previous user by calling the BidBeastsNFTMarket::withdrawAllFailedCredits passing the address of previous user and receiving the funds in the msg.sender address.


Proof of Concept:

  1. Seller list it's NFT on the BidBeastsNFTMarket.

  2. innocentNewUser bids on it which is a smart contract with no fallback or receive function with msg.value of BUY_NOW_PRICE + EXTRA_AMOUNT. Hence ,making the innocentNewUser the owner of the NFT and hence the contract will return extra Eth back .

  3. No fallback or recieve function ,hence making the transaction to fail.

  4. This mapping will be called failedTransferCredits[recipient] += amount;.

  5. BIDDER_1 calls the withdrawAllFailedCredits(address(innocentNewUser)).

  6. EXTRA_AMOUNT will move to BIDDER_1's address after giving a 5% cut.

Proof of Code

Following code will show the Attack.

function setUp() public {
// Deploy contracts
vm.prank(OWNER);
nft = new BidBeasts();
market = new BidBeastsNFTMarket(address(nft));
rejector = new RejectEther();
​
vm.stopPrank();
​
// Fund users
vm.deal(SELLER, STARTING_BALANCE);
vm.deal(BIDDER_1, STARTING_BALANCE);
vm.deal(BIDDER_2, STARTING_BALANCE);
}
​
function test_withdrawAllFailedCredits_anyoneCanGrabFunds() public {
_mintNFT();
_listNFT();
​
InnocentNewUser innocentNewUser = new InnocentNewUser();
uint256 EXTRA_AMOUNT = 5 ether;
​
vm.deal(address(innocentNewUser), BUY_NOW_PRICE + EXTRA_AMOUNT);
​
vm.startPrank(address(innocentNewUser));
market.placeBid{value: BUY_NOW_PRICE + EXTRA_AMOUNT}(0);
vm.stopPrank();
​
vm.prank(BIDDER_1); // the one who is stealing the funds of other users whose transactions were failed
market.withdrawAllFailedCredits(address(innocentNewUser));
​
assert(BIDDER_1.balance > STARTING_BALANCE);
}
contract InnocentNewUser {} // Contract ok innocentNewUser with no fallback or recieve function.

Recommended Mitigation:

  • Inside the BidBeastsNFTMarketPlace::withdrawAllFailedCredits introduce a line of code in the starting checking if the _reciever is equal to msg.sender, hence preventing anyone from withdrawing someone else's ETH .And only the owner of that amount can get it back(incase of gas fee issues).

  • More robust solution can be to introduce a backup account in the Bid struct ,which means adding the parameter address backupAccount.
    So if their main account i.e bidder does not have a recieve or fallback function ,they have an option of receiving the ETH on that backupAccount.
    Changing the logic of the logic accordingly by updating the _executeSale and _payout functions .

...
struct Bid {
address bidder;
uint256 amount;
+ address backupAccount;
}
...
- function placeBid(uint256 tokenId) external payable isListed(tokenId) {
+ function placeBid(uint256 tokenId,address backupAccount) external payable isListed(tokenId) {
...
uint256 previousBidAmount = bids[tokenId].amount;
+ address prevBidderBackup=bids[tokenId].backupAccount;
...
if (listing.buyNowPrice > 0 && msg.value >= listing.buyNowPrice) {
uint256 salePrice = listing.buyNowPrice;
uint256 overpay = msg.value - salePrice;
- bids[tokenId] = Bid(msg.sender, salePrice);
+ bids[tokenId] = Bid(msg.sender, salePrice,backupAccount);
...
}
...
- bids[tokenId] = Bid(msg.sender, msg.value);
+ bids[tokenId] = Bid(msg.sender, msg.value,backupAccount);
​
if (previousBidder != address(0)) {
- _payout(previousBidder, previousBidAmount);
+ _payout(previousBidder, previousBidAmount,prevBidderBackup);
}
​
if (overpay > 0) {
- _payout(msg.sender, overpay);
+ _payout(msg.sender,overpay,backupAccount)
}
​
...
}
...
​
function _executeSale(uint256 tokenId) internal {
Listing storage listing = listings[tokenId];
Bid memory bid = bids[tokenId];
​
listing.listed = false;
delete bids[tokenId];
​
BBERC721.transferFrom(address(this), bid.bidder, tokenId);
​
uint256 fee = (bid.amount * S_FEE_PERCENTAGE) / 100;
s_totalFee += fee;
​
uint256 sellerProceeds = bid.amount - fee;
- _payout(listing.seller, sellerProceeds);
+ _payout(listing.seller, sellerProceeds,address(0));
​
emit AuctionSettled(tokenId, bid.bidder, listing.seller, bid.amount);
}
​
...
- function _payout(address recipient, uint256 amount) internal {
+ function _payout(address recipient, uint256 amount,address backup) internal {
if (amount == 0) return;
(bool success, ) = payable(recipient).call{value: amount}("");
if (!success) {
failedTransferCredits[recipient] += amount;
+ if(backup!=address(0)){
+ failedTransferCredits[backup] += amount
+ }
}
}
...
function withdrawAllFailedCredits(address _receiver) external {
+ if(_receiver!=msg.sender){
+ revert();
+ }
uint256 amount = failedTransferCredits[_receiver];
...
}
...
Updates

Lead Judging Commences

cryptoghost Lead Judge 23 days 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.