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

Errornous `_unstakeVested` function passing wrong parameter to `points.onUnstaked`, skipping unstake of `streamOwner`

bladesec

Summary

The onStreamCanceled function is triggered by the Sablier contract to unstake a stream owner's vested FJORD tokens. It begins by calling _redeem to update the streamOwner’s rewards, then proceeds with _unstakeVested to partially or fully unstake the vested FJORD tokens.

Vulnerability Details

Here's the _unstakeVested where msg.sender is used instead of streamOwner.

function _unstakeVested(address streamOwner, uint256 _streamID, uint256 amount) internal {
NFTData storage data = _streamIDs[streamOwner][_streamID];
DepositReceipt storage dr = deposits[streamOwner][data.epoch];
if (amount > data.amount) revert InvalidAmount();
...
​
> points.onUnstaked(msg.sender, amount);
​
emit VestedUnstaked(streamOwner, epoch, amount, _streamID);
}

As the onStreamCanceled function is always called by the Sablier, it's wrong to use msg.sender in the _unstakeVested function.

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

Hence, the onUnstaked function incorrectly assigns the Sablier address to the user variable, causing the function to revert because the original staking points were correctly assigned to the streamOwner.

Since the onStreamCanceled function is executed within a try-catch block, this revert leads to the points.onUnstaked call being bypassed. Consequently, the user's totalStaked in the FjordPoints contract remains unchanged, enabling them to continue earning points on the unstaked amount and receiving more rewards than they should.

Impact

The explicit streamOwner will gain more rewards than they should, and this can also be used to exploit the system to earn extra rewards.

Tools Used

Manual Review

Recommendation:

To resolve this issue, ensure that the streamOwner is passed to the points.onUnstaked function instead of msg.sender. This will correctly attribute the unstaking to the streamOwner and adjust their points accordingly.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Sablier calls `onStreamCanceled` > `_unstakeVested` > points.onUnstaked(msg.sender, amount); making the effects of points.onUnstaked apply to sablier who's the msg.sender.

Indeed the `points.onUnstaked` should use the streamOwner instead of msg.sender as an input parameter. Impact: high - The vested stakers who got their streams canceled will keep on receiving rewards (points included) for the previously staked stream. Likelihood: low - whenever a Sablier stream sender decides to `cancel()` a recipient's stream

Support

FAQs

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