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

When a Sablier sender cancels a vested stream, the vested stream is not unstaked successfully from `FjordStaking`

3n0ch

Summary

The incorrect input argument of the function call FjordPoints#onUnstaked in FjordStaking#_unstakeVested will cause the vested stream is not unstaked successfully from FjordStaking when a Sablier sender cancels a vested stream.

Vulnerability Details

A Sablier sender can cancel a vested stream by calling SablierV2Lockup#cancel, then the sablier contract will call to FjordStaking#onStreamCanceled

https://github.com/sablier-labs/v2-core/blob/a4bf69cf7024006b9a324eef433f20b74597eaaf/src/SablierV2LockupLinear.sol#L438-L445

function _cancel(uint256 streamId) internal override {
...
if (recipient.code.length > 0) {
>> try ISablierV2LockupRecipient(recipient).onStreamCanceled({
streamId: streamId,
sender: sender,
senderAmount: senderAmount,
recipientAmount: recipientAmount
}) { } catch { }
}
}

FjordStaking#onStreamCanceled will call to FjordStaking#_unstakeVested

https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordStaking.sol#L823-L844

function onStreamCanceled(
uint256 streamId,
address sender,
uint128 senderAmount,
uint128 /*recipientAmount*/
) external override onlySablier checkEpochRollover {
address streamOwner = _streamIDOwners[streamId];
​
if (streamOwner == address(0)) revert StreamOwnerNotFound();
​
_redeem(streamOwner);
​
NFTData memory nftData = _streamIDs[streamOwner][streamId];
​
uint256 amount =
uint256(senderAmount) > nftData.amount ? nftData.amount : uint256(senderAmount);
​
>> _unstakeVested(streamOwner, streamId, amount);
​
emit SablierCanceled(streamOwner, streamId, sender, amount);
}
}

and then FjordPoints#onUnstaked

https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordStaking.sol#L561

function _unstakeVested(address streamOwner, uint256 _streamID, uint256 amount) internal {
...
//INTERACT
if (isFullUnstaked) {
sablier.transferFrom({ from: address(this), to: streamOwner, tokenId: _streamID });
}
​
>> points.onUnstaked(msg.sender, amount);
​
emit VestedUnstaked(streamOwner, epoch, amount, _streamID);
}

But the input argument to the FjordPoints#onUnstaked(address user, uint256 amount) is msg.sender, which is the sablier contract's address. This will cause a revert at the function call FjordPoints#onUnstaked, because the sablier contract is not staking any points, the correct user input argument here should be streamOwner. Since the sablier contract wraps the callback in a try-catch block, the vested stream is cancelled successfully in the sablier contract. However, in the FjordStaking contract, the vested stream is still staked.

Impact

When a Sablier sender cancels a vested stream, the vested stream is not unstaked successfully from FjordStaking, which causes the vested stream is still accruing staking rewards and Fjord points.

Tools Used

Manual Review.

Recommendations

Fix the input argument of the function call FjordPoints#onUnstaked in FjordStaking#_unstakeVested

function _unstakeVested(address streamOwner, uint256 _streamID, uint256 amount) internal {
...
//INTERACT
if (isFullUnstaked) {
sablier.transferFrom({ from: address(this), to: streamOwner, tokenId: _streamID });
}
​
- points.onUnstaked(msg.sender, amount);
+ points.onUnstaked(streamOwner, amount);
​
emit VestedUnstaked(streamOwner, epoch, amount, _streamID);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 10 months 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.