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

Lacking of checking for param `_point` in `FjordPoints::setPointsPerEpoch` function before assigning its value to `pointsPerEpoch` may causing arithmetic overflow and making `FjordPoints` contract can't be used anymore.

kazan

Summary

Lacking of checking for param _point in FjordPoints::setPointsPerEpoch function before assigning its value to pointsPerEpoch may lead to arithmetic overflow in distributePoints function.
And, because the checkDistribution modifier implement the distributePoints function:

modifier checkDistribution() {
distributePoints();
_;
}

Therefore, functions with the checkDistribution modifier will revert.
Unfortunately, almost every function in this contract have the checkDistribution modifier.

Vulnerability Details

The type of pointsPerEpoch variable is uint256, but in distributePoints, the pointsPerToken value is calculated follow:

pointsPerToken = pointsPerToken.add(
weeksPending * (pointsPerEpoch.mul(PRECISION_18).div(totalStaked))
);

The pointsPerEpoch will multiply by PRECISION_18 value (equal to 1e18). So, if the value of pointsPerEpoch is greater than (type(uint256).max / 1e18), the distributePoints function will revert due to arithmetic overflow error. But, the _point param is just checked for zero value, but not checked for the upper bound value before assigning its value to pointsPerEpoch.

Impact

If block.timestamp pass 1 EPOCH_DURATION (7 days). All of this contract functions except setOwner and setStakingContract will revert everytime, making this FjordPoints contract can't be used anymore.

PoC

Paste this test into points.t.sol

function test_setPointsPerEpochTooLargeCauseOverflow() public {
address user = address(0x2);
uint256 stakeAmount = 1000 ether;
fjordPoints.setPointsPerEpoch((type(uint256).max) / 1e18 + 1);
​
vm.startPrank(staking);
fjordPoints.onStaked(user, stakeAmount);
vm.stopPrank();
​
skip(2 weeks);
​
vm.expectRevert();
fjordPoints.distributePoints();
}

Then run forge test --mt test_setPointsPerEpochTooLargeCauseOverflow -vvvv

And the result is:

​
├─ [0] VM::expectRevert(custom error f4844814:)
│ └─ ← [Return]
├─ [1158] FjordPoints::distributePoints()
│ └─ ← [Revert] panic: arithmetic underflow or overflow (0x11)
└─ ← [Stop]
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 412.37µs (79.17µs CPU time)
​
Ran 1 test suite in 8.27ms (412.37µs CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
​

Tools Used

Foundry

Recommendations

Add upper bound checking for _point param.

+ uint256 maxPointsPerEpoch = type(uint256).max / PRECISION_18;
function setPointsPerEpoch(uint256 _points) external onlyOwner checkDistribution {
if (_points == 0) {
revert();
}
+ if (_points >= maxPointsPerEpoch) {
+ revert();
+ }
​
pointsPerEpoch = _points;
}
Updates

Lead Judging Commences

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.