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

Wrong `block.timestamp` comparison in `FjordAuction::bid` and `FjordAuction::auctionEnd` functions, could lead to drained user tokens from the protocol

KaligoAudits

Vulnerability Details

The FjordAuction contract represents an auction, which the admin can create. Users can bid FjordPoints tokens to the auction, using the function bid, so that later when the auction ends, using the auctionEnd function, the user can claim their proportionally calculated tokens of the protocol token, using the function claimTokens. Let's have a deeper look at the bid and auctionEnd functions:

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);
}
function claimTokens() external {
if (!ended) {
revert AuctionNotYetEnded();
}
​
uint256 userBids = bids[msg.sender];
if (userBids == 0) {
revert NoTokensToClaim();
}
​
uint256 claimable = userBids.mul(multiplier).div(PRECISION_18);
bids[msg.sender] = 0;
​
auctionToken.transfer(msg.sender, claimable);
emit TokensClaimed(msg.sender, claimable);
}

As we can see, the bid function checks whether the current timestamp is bigger than the auctionEndTime, which should prevent people from bidding when the time of the auction has passed. The auctionEnd function checks if the current timestamp is less than the auctionEndTime. This way people cannot end an auction if the time has not passed. However, the problem lies in the fact that both functions do not check if the current timestamp is equal to auctionEndTime. With that in mind, we can imagine the following scenario:

  1. A malicious user decides to call bid and auctionEnd functions when the block.timestamp is equal to auctionEndTime.

  2. First, the user calls the auctionEnd function, the auction ends and the multiplier is calculated.

  3. Immediately after, they call the bid function with a big amount. In that way, the user increases the totalBids, without recalculating the multiplier, which is inversely proportional to the totalBids. Since the user has made another bid, his own userBids also increased.

  4. When the user calls the claimTokens function, the multiplier does not reflect the increase in totalBids from the previous action. SinceuserBids was increased without the multiplier being recalculated, the user will claim a larger proportion of the auction prize than he is entitled to.

  5. Although userBids are set to 0 after claimTokens, the user can bid again during the same timestamp, incrementing userBids, allowing him to further drain funds from the auction prize pool.

  6. Since the user can claim a larger proportion of the prize pool, some users would be unable to claim their prizes since there would not be enough tokens in the contract. Further, the malicious user could potentially even claim the whole prize pool.

Impact

A malicious user can cheat the contract, allowing him to claim a larger proportion of the prize pool than he is entitled to. This also impacts other users as some/all would not be able to claim their rewards.

Tools Used

Manual Review

Recommendations

Consider modifying the first if-statement in auctionEnd function:

function auctionEnd() external {
- if (block.timestamp < auctionEndTime) {
+ 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);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 10 months 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.