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

`FjordAuction::auctionEnd` function has a erraneous calculation , causing it to revert when `FjordAuction::totalTokens` is a large value , making users unable to claim their rewards , and tokens are forever stuck in the contract

SoulReaper

Summary

FjordAuction::auctionEnd function has a erraneous calculation , causing it to revert when FjordAuction::totalTokens is a large value , making users unable to claim their rewards , and tokens are forever stuck in the contract

Vulnerability Details

auctionEnd function has the following line:

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

Even though this line uses the SafeMath library of open-zeppelin , if totalTokens.mul(PRECISION_18) overflows , this will revert .
This will revert only if totalTokens is set to a very large value . That value can be calculated as follows

uint a = type(uint256).max;
uint b = 1e18;
uint c = a/b;

Any value greater than c will cause the overflow.

If it reverts , ended flag cannot be set true , and hence FjordAuction::claimTokens can never be called due to the following check

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

Now , this totalTokens value is set in the constructor by whoever is deploying the contract. There is a very low chance the the deployer would be willing to put up so many tokens to be distributed (precisely greater than c as shown above) , but if they do , then the auction can never be completed AND money in the form of 2 tokens , FjordAuction::fjordPoints and FjordAuction::auctionToken , will be stuck in the contract forever.

Proof of code:

  1. A bidder bids in the auction

  2. The auction ends

  3. Somebody calls the auctionEnd function , which reverts

In your auction.t.sol , change your totalTokens value to the following

uint a = type(uint256).max;
uint b = 1e18; // equal to PRECISION_18
uint c = a/b;
uint256 public totalTokens = c+1 ;

And remember to comment out the following line

uint256 public totalTokens = 1000 ether;

And , place the following test into auction.t.sol test suite

function test_auctionEnd_HasMathThatBreaks() public{
address bidder = address(0x2);
uint256 bidAmount = 100 ether;
​
deal(address(fjordPoints), bidder, bidAmount);
​
vm.startPrank(bidder);
fjordPoints.approve(address(auction), bidAmount);
auction.bid(bidAmount);
vm.stopPrank();
​
skip(biddingTime);
​
vm.expectRevert(); // panic error for arithmetic overflow will be triggered
auction.auctionEnd();
}
​

You will also notice that if you change your totalTokens variable to c+1 , 3 of your pre-written tests ALSO FAIL .

Impact

Bidders cannot claim their rewards , and both FjordAuction::fjordPoints and FjordAuction::auctionToken tokens are forever stuck in the contract

Tools Used

Foundry , Manual Review

Recommendations

Best mitigation is to check beforehand whether `totalTokens.mul(PRECISION_18)` will overflow , and if it will , carry out the division before the multiplication , as shown in the following code

function auctionEnd() external {
if (block.timestamp < auctionEndTime) {
revert AuctionNotYetEnded(); // e this function can only be called after the 'deadline'
}
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);
​
+ if (totalTokens > type(uint256).max.div(PRECISION_18)) {
+ multiplier = totalTokens.div(totalBids).mul(PRECISION_18);
+ } else {
+ multiplier = totalTokens.mul(PRECISION_18).div(totalBids);
+ }
​
// Burn the FjordPoints held by the contract
uint256 pointsToBurn = fjordPoints.balanceOf(address(this));
fjordPoints.burn(pointsToBurn);
}

By making this change , you will see that all of your tests in auction.t.sol will pass even with very lage values of totalPoints .
Only one of the tests , auction.t.sol::testAuctionEnd will not pass as it has the same erraneous line , fix it , and then all your tests will pass.

Updates

Lead Judging Commences

inallhonesty Lead Judge
over 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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