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

Users can bid after auction has ended leading to unfair distribution of auction token and DoS

vinica_boy

Summary

In the FjordAuction contract, the auction has a defined end time, stored in the state variable auctionEndTime. Users are allowed to place and withdraw bids until block.timestamp reaches this specified end time. Once this point is reached, anyone can call the auctionEnd() function to finalize the auction. After the auction is finalized, users can claim their auction tokens based on their bids.

However, the current implementation has a vulnerability: users can call auctionEnd() to end the auction and then place additional bids afterwards.

Vulnerability Details

https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordAuction.sol#L143
https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordAuction.sol#L159
https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordAuction.sol#L181

bid() and auctionEnd() have the corresponding checks to see if auction has ended based on the current block.timestamp

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 unbid(uint256 amount) external {
if (block.timestamp > auctionEndTime) {
revert AuctionAlreadyEnded();
}
​
uint256 userBids = bids[msg.sender];
if (userBids == 0) {
revert NoBidsToWithdraw();
}
if (amount > userBids) {
revert InvalidUnbidAmount();
}
​
bids[msg.sender] = bids[msg.sender].sub(amount);
totalBids = totalBids.sub(amount);
fjordPoints.transfer(msg.sender, amount);
emit BidWithdrawn(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);
}
​

Neither the bid() nor the unbid(), nor auctionEnd() functions explicitly check if the current block.timestamp is exactly equal to auctionEndTime. Since the block.timestamp is set by the block miner, it can be adjusted to match the auctionEndTime.

Malicious user could exploit the situation by executing auctionEnd() followed by bid() , claimTokens() and unbid() in the same transaction. During the auctionEnd() call, a multiplier is calculated based on the total bids at that moment. This multiplier is later used to determine the claimable rewards.

This scenario leads to two issues:

  1. The malicious user can obtain a more favorable ratio for their Fjord Points by bidding after auctionEnd() has been called, thereby manipulating the calculated multiplier. This results in an unfair distribution of rewards. Additionally, the malicious user can then withdraw his fjord points by unbid().

  2. After the malicious user claims their auction tokens, the contract may not have enough tokens left to distribute to other legitimate bidders. This imbalance could cause a DoS situation, as other bidders would be unable to claim their rewards until the contract is replenished with additional tokens by the admin.

Impact

As described in the Vulnerability Details section, the impact is unfair distribution of auction tokens and limited DoS.

Tools Used

Manual Review.

Recommendations

Update the checks inside bid() and unbid() functions to also check the ended variable.

function bid(uint256 amount) external {
- if (block.timestamp > auctionEndTime) {
+ if (block.timestamp > auctionEndTime || ended) {
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 unbid(uint256 amount) external {
- if (block.timestamp > auctionEndTime) {
+ if (block.timestamp > auctionEndTime || ended) {
revert AuctionAlreadyEnded();
}
​
uint256 userBids = bids[msg.sender];
if (userBids == 0) {
revert NoBidsToWithdraw();
}
if (amount > userBids) {
revert InvalidUnbidAmount();
}
​
bids[msg.sender] = bids[msg.sender].sub(amount);
totalBids = totalBids.sub(amount);
fjordPoints.transfer(msg.sender, amount);
emit BidWithdrawn(msg.sender, amount);
}
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.