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

When `block.timestamp == auctionEndTime`, a malicious user can bid after `auctionEnd` call, and claim more tokens than it should

y4y

Summary

In the FjordAuction contract, tokens are auctioned by points, reward for auctioned tokens are calculated based on total bid points. The time check in handling bidding, unbidding and auction ending is faulty, which makes a malicious user to bid even after auctionEnd has been called, and get more auction tokens than deserved.

Vulnerability Details

We see that in bid function, users cannot bid anymore when the timestamp exceeds auctionEndTime:

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);
}

And similar logic is implemented here in auctionEnd:

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);
}

However, when block.timestamp == auctionEndTime, both function can be called, regardless of calling order. Also, we notice that, during reward distribution, multiplier variable is used to determine the final amount:

​
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);
}

So consider in a scenario, where we have block.timestamp == auctionEndTime, and at this moment, the total bids is 20, and total tokens is at 1000, for demonstration purpose. Now, Alice comes in, calls auctionEnd, and sets multiplier to 1000 / 20 * 1e18, making it 50 * 1e18. Then in the very next trnsaction, Alice bids another 20 points, the bid would go through, as the function does not check if the auction has ended, this will not change the multiplier, but will increase Alice's bidding balance. Next, Alice calls claimTokens, based on the calculation, she would get 20 * 50 * 1e18 / 1e18 = 1000 auction tokens, which is the entire token supply, leaving other users to unable to claim their deserved tokens.

Impact

All auction tokens can potentially be claimed, and also leaving other legit users to lose their rewards. Average block time is about 12 seconds, the change of this happening is quite high, combined with loss/steal of funds, this should be high severity.

Tools Used

Manual review

Recommendations

Change the timestamp comparison, also add additional check on auction end status.

Updates

Lead Judging Commences

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