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

Usage of `transferFrom` can lead to funds lost

bronzepickaxe

Summary

Due to the usage of transferFrom instead of withdrawMaxAndTransfer, funds can be lost.

Vulnerability Details

transferFrom is used inside FjordStaking.stakeVested():

function stakeVested(uint256 _streamID) external checkEpochRollover redeemPendingRewards {
//CHECK
if (!sablier.isStream(_streamID)) revert NotAStream();
if (sablier.isCold(_streamID)) revert NotAWarmStream();
​
// only allow authorized stream sender to stake cancelable stream
if (!authorizedSablierSenders[sablier.getSender(_streamID)]) {
revert StreamNotSupported();
}
if (address(sablier.getAsset(_streamID)) != address(fjordToken)) revert InvalidAsset();
​
uint128 depositedAmount = sablier.getDepositedAmount(_streamID);
uint128 withdrawnAmount = sablier.getWithdrawnAmount(_streamID);
uint128 refundedAmount = sablier.getRefundedAmount(_streamID);
​
if (depositedAmount - (withdrawnAmount + refundedAmount) <= 0) revert InvalidAmount();
​
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;
​
//INTERACT
-> sablier.transferFrom({ from: msg.sender, to: address(this), tokenId: _streamID });
points.onStaked(msg.sender, _amount);
​
emit VestedStaked(msg.sender, currentEpoch, _streamID, _amount);
}
​

If we take a look at the Sablier Docs, we find the following warning:

Be careful with ` transferFrom`.
All remaining funds, including the already streamed portion, will enter into the possession of the new recipient.
Consider using ` withdrawMaxAndTransfer ` instead.

This means in the current implementation, when a user has an already streamed portion, he will lose this portion when transferring the NFT to the FjordStaking.sol contract.

Impact

The usage of transferFrom will lead to loss of funds due to the fact that the streamed portion will be lost.

Tools Used

Manual Review

Recommendations

Use withdrawMaxAndTransfer

Updates

Lead Judging Commences

inallhonesty Lead Judge
10 months ago
inallhonesty Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Appeal created

bronzepickaxe Submitter
9 months ago
inallhonesty Lead Judge
9 months ago
bronzepickaxe Submitter
9 months ago
bronzepickaxe Submitter
9 months ago
inallhonesty Lead Judge
9 months ago
inallhonesty Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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