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

Incorrect stake reduction in `FjordPoints` on Sablier stream cancellation

irondevx

Summary

When a Sablier stream is cancelled, the onStreamCanceled function in FjordStaking calls _unstakeVested, which reduces the stake in the FjordPoints contract. However, it uses msg.sender (the Sablier contract) instead of the actual stream owner, leading to incorrect stake accounting and potential issues in the points system.

Impact

  • The wrong address (Sablier contract) will have its stake reduced in the FjordPoints contract, while the actual user's stake remains unchanged.

  • However, since the Sablier contract address doesn't exist in the FjordPoints contract's users mapping, the onUnstaked function might be DoS'd, preventing stream cancellations.

Proof of Concept

Here's a full flow of the Sablier stream cancellation:

FjordStaking.sol#L840

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

FjordStaking.sol#L561

function _unstakeVested(address streamOwner, uint256 _streamID, uint256 amount) internal {
// existing code...
​
//INTERACT
// 6) Return Sablier NFT
if (isFullUnstaked) {
sablier.transferFrom({ from: address(this), to: streamOwner, tokenId: _streamID });
}
​
// 7) Reduce user's stake in points contract
@> points.onUnstaked(msg.sender, amount); // <---- msg.sender is actually Sablier
​
emit VestedUnstaked(streamOwner, epoch, amount, _streamID);
}

FjordPoints.sol#L220

function onUnstaked(address user, uint256 amount)
external
onlyStaking
checkDistribution
updatePendingPoints(user)
{
@> UserInfo storage userInfo = users[user]; // <----- might even DoS because Sablier's sender doesn't exist here
if (amount > userInfo.stakedAmount) {
revert UnstakingAmountExceedsStakedAmount();
}
userInfo.stakedAmount = userInfo.stakedAmount.sub(amount);
totalStaked = totalStaked.sub(amount);
emit Unstaked(user, amount);
}

Tools Used

Manual code review

Recommended Mitigation Steps

Modify the FjordStaking::_unstakeVested function to accept the stream owner's address as a parameter, and pass this address to the points.onUnstaked call.

function _unstakeVested(address streamOwner, uint256 _streamID, uint256 amount) internal {
// existing code...
​
//INTERACT
// 6) Return Sablier NFT
if (isFullUnstaked) {
sablier.transferFrom({ from: address(this), to: streamOwner, tokenId: _streamID });
}
​
// 7) Reduce user's stake in points contract
- points.onUnstaked(msg.sender, amount);
+ points.onUnstaked(streamOwner, amount);
​
emit VestedUnstaked(streamOwner, epoch, amount, _streamID);
}
Updates

Lead Judging Commences

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