Part 2
Zaros
PerpetualsDEXFoundrySolidity
70,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Users loose claimable fees when they add a stake to an existing stake.

kirkeelee

Summary

Due to the way new stake is handled, users loose claimable fees when they add a stake to an existing staked amount.

Vulnerability Details

When a user stakes, accumulateActor is called within the stake function of VaultRouterBranch.sol. The issue is that the accumulateActor does not do what the name of the function implies.

function accumulateActor(Data storage self, bytes32 actorId) internal returns (SD59x18 valueChange) {
Actor storage actor = self.actor[actorId];
return _updateLastValuePerShare(self, actor, ud60x18(actor.shares));
}

As can be seen from the code above, it only updates the LastValuePerShare for the user. The earned fees, valueChange, are not stored anywhere and user's LastValuePerShare is set to the latest valuepershare. Therefore, in the case of user having a previous stake with a different valuepershare compared to the current one(has earned fees) and adding a new stake will lead to user loosing the previously earned fees. Ideally, the code should revert if there are unclaimed fees for the user as it is done in the unstake function.

This issue can be observed from the below POC. To run it, add the below test to the claimFees.t.soland run with forge test --match-test testFuzz_LostClaimablesOnAdditionalStake -vvv command.

function testFuzz_LostClaimablesOnAdditionalStake(
uint128 assetsToDeposit,
uint128 additionalDeposit,
uint128 marketFees
) external {
// Setup
uint128 vaultId = WETH_CORE_VAULT_ID;
VaultConfig memory fuzzVaultConfig = getFuzzVaultConfig(vaultId);
address user = users.naruto.account;
// First deposit & stake
assetsToDeposit = uint128(bound(assetsToDeposit, calculateMinOfSharesToStake(vaultId), fuzzVaultConfig.depositCap / 3));
fundUserAndDepositInVault(user, vaultId, assetsToDeposit);
​
ClaimFeesState memory preStakeState = _getClaimFeesState(
user,
vaultId,
IERC20(fuzzVaultConfig.asset),
IERC20(fuzzVaultConfig.indexToken)
);
​
vm.startPrank(user);
marketMakingEngine.stake(vaultId, preStakeState.stakerVaultBal);
vm.stopPrank();
// Generate fees
marketFees = uint128(bound(marketFees, 1e16, 1e20));
deal(fuzzVaultConfig.asset, address(perpMarketsCreditConfig[ETH_USD_MARKET_ID].engine), marketFees);
changePrank(address(perpMarketsCreditConfig[ETH_USD_MARKET_ID].engine));
marketMakingEngine.receiveMarketFee(ETH_USD_MARKET_ID, fuzzVaultConfig.asset, marketFees);
// Check earned fees
uint256 earnedFeesBeforeSecondStake = marketMakingEngine.getEarnedFees(vaultId, user);
assertGt(earnedFeesBeforeSecondStake, 0, "Should have earned fees");
// Second deposit & stake
additionalDeposit = uint128(bound(additionalDeposit, calculateMinOfSharesToStake(vaultId), fuzzVaultConfig.depositCap / 3));
fundUserAndDepositInVault(user, vaultId, additionalDeposit);
​
ClaimFeesState memory preSecondStakeState = _getClaimFeesState(
user,
vaultId,
IERC20(fuzzVaultConfig.asset),
IERC20(fuzzVaultConfig.indexToken)
);
​
vm.startPrank(user);
marketMakingEngine.stake(vaultId, preSecondStakeState.stakerVaultBal);
vm.stopPrank();
// Verify fees lost after second stake
uint256 earnedFeesAfterSecondStake = marketMakingEngine.getEarnedFees(vaultId, user);
assertEq(earnedFeesAfterSecondStake, 0, "Earned fees should be zero after second stake");
}

Impact

Users loose claimable fees when they stake additional index tokens on top of previous stake.

Tools Used

Foundry

Recommendations

Make the stake operation to revert when there are claimable rewards as it is done in during unstaking.

if (!amountToClaimX18.isZero()) revert Errors.UserHasPendingRewards(actorId, amountToClaimX18.intoUint256());
​
Updates

Lead Judging Commences

inallhonesty Lead Judge 6 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Inside VaultRouterBranch if you stake wait some time then stake again makes you lose the rewards.

Support

FAQs

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