Error in Staking.sol unstaking calculation which could lead to users unstaking nothing instead of 20%

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Staking.sol#L64 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Staking.sol#L198 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Staking.sol#L210 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingConfig.sol#L18

Vulnerability details

Impact

In Staking.sol a user who has already staked a certain amount of salt, can choose to place a pending transaction to claim the salt staked. However should the user wish to unstake their token before the 52 week duration period then the user loses a portion of their stake.

The maximum amount of tokens a user is suppose to lose is 80% of their initial stake amount, should they wish to wait a minimum of two weeks according to the current set staking config, to complete the unstaking transaction.

However there is an error in the function to calculate the users unstake amount which results in a user losing all their staked tokens should they choose to wait for the minimum unstake waiting time.

Proof of Concept

The function calculateUnstake:

First collects these variable from StakingConfig.sol results for calls already substituted

uint256 minUnstakeWeeks = 2 uint256 maxUnstakeWeeks = 52; uint256 minUnstakePercent = 20;

Then has two require statement to check if the number of weeks supplied by the user is within the acceptable range, as there are no checks is unstake

require( numWeeks >= minUnstakeWeeks, "Unstaking duration too short" );
require( numWeeks <= maxUnstakeWeeks, "Unstaking duration too long" );

However the first check is where the error emanates from as we will see that when the numWeeks is equal to minUnstakeWeeks then we get a zero for the lines

uint256 numerator = unstakedXSALT * ( minUnstakePercent * unstakeRange + percentAboveMinimum * ( numWeeks - minUnstakeWeeks ) );
return numerator / ( 100 * unstakeRange );

mainly because numWeeks - minUnstakeWeek is equal to zero.

As such the numerator will equate to zero.

The problem is that in the unstake function, the user claimable salt will be zero and yet they will have unstaked all their salt. As such a user waits to weeks to claim nothing.

Tools Used

Manual Review

Recommended Mitigation Steps

There are two possible fixes, the first is to change the require statement to be strictly greater than the minimum number of weeks and the second id to increase the numWeeks - minUnstakeWeek calculation by 1. Which ever aligns with the protocols intended calculation goals could be superior. A final option is to add a custom function sub routine to deal with cases where the minimum number of weeks is used.

Assessed type

Math

[Picodes]

What about the minUnstakePercent * unstakeRange part?

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid