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

`msg.sender` is used instead of `streamOwner` while unstaking from pointsContract in _unstakeVested()

0xaraj

Summary

msg.sender is used instead of streamOwner while unstaking from pointsContract in _unstakeVested()

Vulnerability Details

fjordStaking:_unstakeVested() is an internal function, which is used for unstaking vested tokens. It takes streamOwner/ streamID/ amount as input parameters. But while unstaking from pointsContract, it passes msg.sender instead of streamOwner(which is taken as input)

function _unstakeVested(address streamOwner, uint256 _streamID, uint256 amount) internal {
...
@> points.onUnstaked(msg.sender, amount);
}

Now this a problem because, when user stakeVested then points.onStake() is called with msg.seder, who is owner of streamID ie streamOwner

function stakeVested(uint256 _streamID) external checkEpochRollover redeemPendingRewards {
...
//INTERACT
sablier.transferFrom({ from: msg.sender, to: address(this), tokenId: _streamID });
@> points.onStaked(msg.sender, _amount);
}

Now, _unstakeVested() is also called inside fjordStaking:onStreamCanceled(), which can only be called by sablier due to onlySablier modifier.

function onStreamCanceled(
uint256 streamId,
address sender,
uint128 senderAmount,
uint128 /*recipientAmount*/
) external override onlySablier checkEpochRollover {
...
@> _unstakeVested(streamOwner, streamId, amount);
}

In onStreamCanceled(), msg.sender of _unstakeVested() will be sablier & points.onUnstaked() will be called with msg.sender(sablier) & in above code we saw points.onStaked() is called with msg.sender(who is owner of that streamID).

As result, points.onUnstaked() will always revert in _unstakeVested() when called from onSteamCanceled() because there is no stake in pointsContract with sablier address, but with the owner of streamID on whome onSteamCanceled() is called.

Natspecs of onStreamCanceled() says, this function may revert but it will always revert

/// @dev Notes: This function may revert, but the Sablier contract will ignore the revert.

Even after passsing correct inputs in _unstakeVested() but due to using msg.sender instead of streamOwner while unstaking from pointsContract will always revert the trx.

Impact

onStreamCanceled() will always revert

Tools Used

Manual Review

Recommendations

Use steamOwer instead of msg.sener in _unstakeVested() while unstaking from pointsContract

- points.onUnstaked(msg.sender, amount);
+ points.onUnstaked(steamOwner, amount);
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.