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

Flawed handling of Sablier NFT cancellations in fjord staking allows indefinite reward accumulation on refunded assets

0xbrivan2

Summary

The Fjord Staking contract does not correctly process the cancellation of Sablier vesting NFTs, enabling users to indefinitely earn rewards on refunded assets from canceled streams. This occurs due to a silent revert in the unstaking process, caused by an incorrect call to FjordPoints.onUnstake, allowing rewards to accumulate on funds that have already been refunded.

Vulnerability Details

The stakeVested function in the Fjord Staking contract allows users to stake their Sablier vesting NFTs, which hold FJORD tokens, to earn rewards. When a stream is canceled, the onStreamCanceled hook is supposed to handle the cancellation by unstaking the refunded assets to stream sender (the unstreamed amount). However, the current implementation always reverts silently due to an incorrect call to FjordPoints within _unstakeVested execution.

function onStreamCanceled(uint256 streamId, address sender, uint128 senderAmount, uint128
) external override onlySablier checkEpochRollover {
// --SNIP
uint256 amount = uint256(senderAmount) > nftData.amount ? nftData.amount : uint256(senderAmount);
>>> _unstakeVested(streamOwner, streamId, amount);
emit SablierCanceled(streamOwner, streamId, sender, amount);
}

After the onStreamCanceled function handles necessary checks, it calls _unstakeVested to unstake the amount of assets being refunded to the stream's sender and reduct that amount from the staked balance. The function _unstakeVested calls points.onUnstaked, which triggers an underflow revert due to an incorrect msg.sender being passed, as shown below:

function _unstakeVested(address streamOwner, uint256 _streamID, uint256 amount) internal {
// --SNIP
>>> points.onUnstaked(msg.sender, amount); // @audit msg.sender corresdponds to Sablier contract
emit VestedUnstaked(streamOwner, epoch, amount, _streamID);
}

The msg.sender passed to points.onUnstaked refers to Sablier and this is incorrect because the original staker was not the Sablier contract. Since the Sablier contract does not have any stakes, the call reverts with UnstakingAmountExceedsStakedAmount:

function onUnstaked(address user, uint256 amount) external onlyStaking checkDistribution updatePendingPoints(user) {
UserInfo storage userInfo = users[user];
if (amount > userInfo.stakedAmount) {
>>> revert UnstakingAmountExceedsStakedAmount();
}
​
// --SNIP
}

However, the transaction as a whole does not revert because the Sablier version used (v1.0.2) gracefully handles hook failures

function _cancel(uint256 streamId) internal override {
// --SNIP
// Interactions: if `msg.sender` is the sender and the recipient is a contract, try to invoke the cancel
// hook on the recipient without reverting if the hook is not implemented, and without bubbling up any
// potential revert.
if (msg.sender == sender) {
if (recipient.code.length > 0) {
try ISablierV2LockupRecipient(recipient).onStreamCanceled({
streamId: streamId,
sender: sender,
senderAmount: senderAmount,
recipientAmount: recipientAmount
>>> }) { } catch { }
}
}
​
// --SNIP
}

As a result, the stream's sender successfully cancels the stream and is refunded, while the staker (recipient) continues earning rewards for the refunded amount indefinitely because his stakes states were not updated.

Proof Of Concept

The following test case demonstrates how canceling streams will always silently revert the unstaking process on the protocol. Copy and paste the following test case into test/unit/stakeVested.t.sol:

function test_cancelStreamsNotHandledProperly() public {
// Setting up the contracts
FjordPoints fjordPoints = new FjordPoints();
fjordStaking = new FjordStaking(address(token), address(minter), address(SABLIER), address(this), address(fjordPoints));
fjordPoints.setStakingContract(address(fjordStaking));
vm.prank(minter);
token.approve(address(fjordStaking), 100 ether);
​
​
// Create the stream
uint amount = 1000 ether;
deal(address(token), address(this), amount);
token.approve(address(SABLIER), amount);
​
LockupLinear.CreateWithRange memory params;
​
params.sender = address(this);
params.recipient = alice;
params.totalAmount = uint128(amount);
params.asset = IERC20(address(token));
params.cancelable = true;
params.range = LockupLinear.Range({
start: uint40(vm.getBlockTimestamp()),
cliff: uint40(vm.getBlockTimestamp()),
end: uint40(vm.getBlockTimestamp() + 20 days)
});
params.broker = Broker(address(0), ud60x18(0));
uint streamID = SABLIER.createWithRange(params);
​
// Alice stakes her Sablier NFT
vm.startPrank(alice);
SABLIER.approve(address(fjordStaking), streamID);
fjordStaking.stakeVested(streamID);
vm.stopPrank();
​
// The stream is canceled on the same epoch Alice staked
// The sender will withdraw all the NFT amount since it is canceled just after creation
vm.prank(address(this));
SABLIER.cancel(streamID);
​
// 3 epochs are passed and rewards are added
_addRewardAndEpochRollover(10 ether, 3);
​
// Even tho the stream was canceled, alice will continue earning rewards
vm.startPrank(alice);
fjordStaking.claimReward(false);
(,uint256 claimAmount) = fjordStaking.claimReceipts(address(alice));
assertEq(30 ether, claimAmount);
​
// Another 4 epochs passed and rewards are added
_addRewardAndEpochRollover(10 ether, 4);
vm.startPrank(alice);
​
// Complete claim request from last one
fjordStaking.completeClaimRequest();
​
// Alice will keep earning more rewards
fjordStaking.claimReward(false);
(, claimAmount) = fjordStaking.claimReceipts(address(alice));
assertEq(40 ether, claimAmount);
​
}

Note: The contracts were reinitialized at the start of the test case because FjordStakingBase only mocks calls to the staking contract

To see the silent function revert trace, run the test with forge test -vvvv --mt test_cancelStreamsNotHandledProperly:

[45799] FjordStaking::onStreamCanceled(1402, StakeVestedTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 1000000000000000000000 [1e21], 0)
│ │ ├─ [24515] 0xB10daee1FCF62243aE27776D7a92D39dC8740f95::transferFrom(FjordStaking: [0xc7183455a4C133Ae270771860664b6B7ec320bB1], alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6], 1402)
│ │ │ ├─ emit Transfer(from: FjordStaking: [0xc7183455a4C133Ae270771860664b6B7ec320bB1], to: alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6], tokenId: 1402)
│ │ │ └─ ← ()
>>> │ │ ├─ [8365] FjordPoints::onUnstaked(0xB10daee1FCF62243aE27776D7a92D39dC8740f95, 1000000000000000000000 [1e21])
>>> │ │ │ └─ ← UnstakingAmountExceedsStakedAmount()
│ │ └─ ← UnstakingAmountExceedsStakedAmount()
│ ├─ emit CancelLockupStream(streamId: 1402, sender: StakeVestedTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], recipient: FjordStaking: [0xc7183455a4C133Ae270771860664b6B7ec320bB1], senderAmount: 1000000000000000000000 [1e21], recipientAmount: 0)

Impact

Canceled streams are not handled correctly, allowing stakers to indefinitely earn rewards on refunded assets of canceled Sablier NFTs

Tools Used

Manual Review

Recommendations

Modify the _unstakeVested function to pass the streamOwner to the points.onUnstake instead of msg.sender:

function _unstakeVested(address streamOwner, uint256 _streamID, uint256 amount) internal {
// --SNIP
​
- points.onUnstaked(msg.sender, amount);
+ points.onUnstaked(streamOwner, amount);
​
emit VestedUnstaked(streamOwner, epoch, amount, _streamID);
}
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.