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

During a full unstake, the NFT is transferred back to the owner while points updates only occur for partial unstakes

daniel526

Summary

See bellow

Vulnerability Details

In the _unstakeVested function of the staking contract, the handling of full and partial unstakes appears inconsistent with respect to how points are updated in the FjordPoints contract. The function processes both full and partial unstakes using the same logic, which can lead to incorrect points calculations.

Code Snippet:

if (isFullUnstaked) {
sablier.transferFrom({ from: address(this), to: streamOwner, tokenId: _streamID });
}
​
points.onUnstaked(msg.sender, amount);

  • Full Unstake: When isFullUnstaked is true, the function transfers the NFT back to the owner. However, it still calls the onUnstaked function with the amount parameter, which is intended for partial unstakes.

  • Partial Unstake: The onUnstaked function processes the amount and updates the user’s staked balance and pending points.
    Relevant onUnstaked Function in FjordPoints Contract:

/**
* @dev Modifier to update pending points for a user.
* @param user The address of the user to update points for.
*/
modifier updatePendingPoints(address user) {
UserInfo storage userInfo = users[user];
uint256 owed = userInfo.stakedAmount.mul(pointsPerToken.sub(userInfo.lastPointsPerToken))
.div(PRECISION_18);
userInfo.pendingPoints = userInfo.pendingPoints.add(owed);
userInfo.lastPointsPerToken = pointsPerToken;
_;
}
​
/**
* @notice Records the amount of tokens unstaked by a user.
* @param user The address of the user unstaking tokens.
* @param amount The amount of tokens being unstaked.
*/
function onUnstaked(address user, uint256 amount)
external
onlyStaking
checkDistribution
updatePendingPoints(user)
{
UserInfo storage userInfo = users[user];
if (amount > userInfo.stakedAmount) {
revert UnstakingAmountExceedsStakedAmount();
}
userInfo.stakedAmount = userInfo.stakedAmount.sub(amount);
totalStaked = totalStaked.sub(amount);
emit Unstaked(user, amount);
}
​

When a full unstake is executed:

  • The NFT is transferred back to the user.

  • The onUnstaked function is invoked with the amount parameter, which is intended for partial unstaking.
    This creates a mismatch:

  • For full unstake: The amount parameter should logically be irrelevant since the entire NFT (and thus the entire stake) is being returned. However, the onUnstaked function adjusts the staked balance as if it were a partial unstake.

Impact

  • The staked balance might be incorrectly reduced if the amount used in the onUnstaked function is not accurate. This could leave the user's staked balance in an inconsistent state.

  • The pending points calculation might be skewed because the onUnstaked function updates points based on the amount parameter. For full unstakes, this could mean points are calculated or left pending inaccurately.

Tools Used

Manual

Recommendations

separate the logic for full unstake into its own function or conditional handling that ensures correct points adjustment when transferring the NFT:

if (isFullUnstaked) {
sablier.transferFrom({ from: address(this), to: streamOwner, tokenId: _streamID });
// Adjust points update or handle full unstake separately
handleFullUnstake(user);
} else {
onUnstaked(user, amount);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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