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

Fjord Incorrectly Assumes the Same Amount of Tokens Are Still Vested After Withdrawal on Sablier

0xaman

Summary

The Fjord Protocol allows users to stake linear stream vested tokens from Sablier. It calculates the staking token amount when the vesting tokens are initially added to the protocol. However, the protocol does not implement a withdraw hook from Sablier, so it assumes the vested token amount in Sablier remains unchanged. Consequently, rewards are distributed to the NFT owner based on the initially calculated amount, even if tokens are later withdrawn from Sablier. Keep in mind that the sender of the stream in Sablier can call the withdraw function. Refer to the subsequent section for more details.

Vulnerability Details

Let have a Look at the withdraw function of Sbailer@V1.1.2 :

function withdraw(
uint256 streamId,
address to,
uint128 amount
)
public
override
noDelegateCall
updateMetadata(streamId)
{
// Checks: the stream is not depleted. This also checks that the stream is not null.
if (isDepleted(streamId)) {
revert Errors.SablierV2Lockup_StreamDepleted(streamId);
}
​
@1> bool isCallerStreamSender = _isCallerStreamSender(streamId);
​
// Checks: `msg.sender` is the stream's sender, the stream's recipient, or an approved third party.
@2> if (!isCallerStreamSender && !_isCallerStreamRecipientOrApproved(streamId)) {
revert Errors.SablierV2Lockup_Unauthorized(streamId, msg.sender);
}
​
// Retrieve the recipient from storage.
@3> address recipient = _ownerOf(streamId);
​
// Checks: if `msg.sender` is the stream's sender, the withdrawal address must be the recipient.
if (isCallerStreamSender && to != recipient) {
revert Errors.SablierV2Lockup_InvalidSenderWithdrawal(streamId, msg.sender, to);
}
​
// Checks: the withdrawal address is not zero.
if (to == address(0)) {
revert Errors.SablierV2Lockup_WithdrawToZeroAddress();
}
​
// Checks: the withdraw amount is not zero.
if (amount == 0) {
revert Errors.SablierV2Lockup_WithdrawAmountZero(streamId);
}
​
// Checks: the withdraw amount is not greater than the withdrawable amount.
uint128 withdrawableAmount = _withdrawableAmountOf(streamId);
if (amount > withdrawableAmount) {
revert Errors.SablierV2Lockup_Overdraw(streamId, amount, withdrawableAmount);
}
​
// Effects and Interactions: make the withdrawal.
@4> _withdraw(streamId, to, amount);
​
// Interactions: if `msg.sender` is not the recipient and the recipient is a contract, try to invoke the
// withdraw hook on it without reverting if the hook is not implemented, and also without bubbling up
// any potential revert.
if (msg.sender != recipient && recipient.code.length > 0) {
try ISablierV2LockupRecipient(recipient).onStreamWithdrawn({
streamId: streamId,
caller: msg.sender,
to: to,
amount: amount
}) { } catch { }
}
}
​

In the code snippet, at @1>, the code checks if the caller is the stream sender. Then, at @2>, it validates whether the caller is either the sender, recipient, or an approved recipient. If this validation passes, it is considered a valid call. The withdraw function than send the token to the recipient which in our case will be staking contract.

The issue is that the Fjord staking contract does not implement the onStreamWithdrawn hook, which falsely assumes that the stream still holds the exact same amount of tokens as it did initially.

Let break down this case in following steps :

  1. Bob has a stream with an available amount = 100 ether and NFT = 1.

  2. Bob calls stakeVested with streamId = 1.

  3. Fjord calculates the available amount and creates deposits with 100 ether for the currentEpoch.

@>----- uint256 _amount = depositedAmount - (withdrawnAmount + refundedAmount);
​
//EFFECT
userData[msg.sender].unredeemedEpoch = currentEpoch;
​
DepositReceipt storage dr = deposits[msg.sender][currentEpoch];
if (dr.epoch == 0) {
@>-------- dr.vestedStaked = _amount;
dr.epoch = currentEpoch;
​
_activeDeposits[msg.sender].add(currentEpoch);
} else {
dr.vestedStaked += _amount;
}
​
_streamIDs[msg.sender][_streamID] = NFTData({ epoch: currentEpoch, amount: _amount });
_streamIDOwners[_streamID] = msg.sender;
newStaked += _amount;
newVestedStaked += _amount;

  1. From this point onward, the rewards for this staking will be calculated based on _amount = 100 ether, regardless of whether the sender calls the withdraw function on Sablier.

function _redeem(address sender) internal {
//1. Get user data
UserData storage ud = userData[sender];
​
ud.unclaimedRewards +=
calculateReward(ud.totalStaked, ud.lastClaimedEpoch, currentEpoch - 1);
ud.lastClaimedEpoch = currentEpoch - 1;
​
if (ud.unredeemedEpoch > 0 && ud.unredeemedEpoch < currentEpoch) {
// 2. Calculate rewards for all deposits since last redeemed, there will be only 1 pending unredeemed epoch
DepositReceipt memory deposit = deposits[sender][ud.unredeemedEpoch];
​
// 3. Update last redeemed and pending rewards
ud.unclaimedRewards += calculateReward(
deposit.staked + deposit.vestedStaked, ud.unredeemedEpoch, currentEpoch - 1
);
// console.log("ud.unclaimedRewards" , ud.unclaimedRewards);
​
ud.unredeemedEpoch = 0;
@> ud.totalStaked += (deposit.staked + deposit.vestedStaked); // this totalStaked will be used for rewards calculation.
}
​

  1. The sender withdraws 50 tokens from the stream on Sablier. However, due to the absence of an onStreamWithdrawn hook implementation, Fjord will still assume that the stream holds 100 tokens. In reality, the stream now contains fewer tokens, which can result in a loss for the Fjord staking contract. This is because Fjord relies on the stream holding the full 100 tokens and accumulates rewards based on this assumption.

Note: Not in scope for this contest and not be applied to this report

I suggest that if Fjord decides to use Sablier's latest version (1.2.2), anyone can call the withdraw function as long as the recipient is valid. Therefore, if the team plans to use the latest version in the future, they must implement the onStreamWithdrawn hook. Version@1.2.2.

Impact

Fjord Staking will calculate incorrect rewards for vested stakes if tokens are withdrawn from the stream on Sablier.

Tools Used

Manual review

Recommendations

Implement the onStreamWithdrawn hook with the following considerations:

  1. Update the user's staked amount based on the received amount in the hook. Subtract this amount from the user's totalStaked.

  2. If the remaining amount after subtraction is zero, delete the staking record and transfer the NFT back to the owner.

Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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