Summary
An issue exists in the VaultRouterBranch contract where calling the stake() function while having unclaimed earned fees results in the loss of accumulated fees. VaultRouterBranch.stake() lacks a validation check for unclaimed rewards, potentially causing unintentional loss of user funds.
Vulnerability Details
Users who have previously deposited funds into vaults can stake their vault's index tokens to earn fees from that vault. When VaultRouterBranch.stake() or VaultRouterBranch.unstake() are called, wethRewardDistribution.accumulateActor() is executed, which triggers the recalculation of the user's earned fees. However, if there are unclaimed earned fees, the call to wethRewardDistribution.accumulateActor() will overwrite those values, causing the user to lose their unclaimed earned fees from staking.
The VaultRouterBranch.unstake() function checks for this issue (see code snippet below) and prevents users from calling unstake() with unclaimed fees. However, stake() does not have this check and therefore allows users to call it with unclaimed earned fees, unintentionally resetting their staking rewards. Note that neither the documentation nor the code natspec contains any warning about calling stake() with unclaimed earned fees.
UD60x18 amountToClaimX18 = vault.wethRewardDistribution.getActorValueChange(actorId).intoUD60x18();
if (!amountToClaimX18.isZero()) revert Errors.UserHasPendingRewards(actorId, amountToClaimX18.intoUint256());
Check the POC below (Copy the code to a file in test/integration/market-making/fee-distribution-branch/claimFees and run forge build && forge test --mt testStakePOC):
pragma solidity 0.8.25;
import { Base_Test } from "test/Base.t.sol";
import { Errors } from "@zaros/utils/Errors.sol";
import { Constants } from "@zaros/utils/Constants.sol";
import { Math } from "@zaros/utils/Math.sol";
import { FeeDistributionBranch } from "@zaros/market-making/branches/FeeDistributionBranch.sol";
import { IDexAdapter } from "@zaros/utils/interfaces/IDexAdapter.sol";
import { UD60x18, ud60x18 } from "@prb-math/UD60x18.sol";
import { sd59x18 } from "@prb-math/SD59x18.sol";
import { IERC20 } from "@openzeppelin/token/ERC20/IERC20.sol";
contract ClaimFees_Integration_Test is Base_Test {
function setUp() public virtual override {
Base_Test.setUp();
changePrank({ msgSender: address(users.owner.account) });
createVaults(marketMakingEngine, INITIAL_VAULT_ID, FINAL_VAULT_ID, true, address(perpsEngine));
configureMarkets();
}
function testStakePOC() external {
uint128 vaultId = WETH_CORE_VAULT_ID;
VaultConfig memory fuzzVaultConfig = getFuzzVaultConfig(vaultId);
address user = users.naruto.account;
uint128 assetsToDeposit = 2 * uint128(calculateMinOfSharesToStake(vaultId));
fundUserAndDepositInVault(user, vaultId, assetsToDeposit);
vm.prank(user);
marketMakingEngine.stake(vaultId, 100000);
uint128 marketFees = uint128(calculateMinOfSharesToStake(vaultId));
deal(fuzzVaultConfig.asset, address(perpMarketsCreditConfig[ETH_USD_MARKET_ID].engine), marketFees);
changePrank({ msgSender: address(perpMarketsCreditConfig[ETH_USD_MARKET_ID].engine) });
marketMakingEngine.receiveMarketFee(ETH_USD_MARKET_ID, fuzzVaultConfig.asset, marketFees);
uint256 rewardsBeforeSecondStaking = marketMakingEngine.getEarnedFees(vaultId, user);
assertGt(rewardsBeforeSecondStaking, 0);
vm.startPrank(user);
marketMakingEngine.stake(vaultId, 100000);
uint256 rewardsAfterSecondStaking = marketMakingEngine.getEarnedFees(vaultId, user);
assertEq(rewardsAfterSecondStaking, 0);
}
}
Impact
Calling VaultRouterBranch.stake() with unclaimed earned fees will result in permanent loss of those unclaimed fees for that user.
Tools Used
Manual Review
Recommended Mitigation
Consider adding the same !amountToClaimX18.isZero() check to VaultRouterBranch.stake() that is already present in VaultRouterBranch.unstake(). See the diff below for an example implementation of this fix.
@@ -379,48 +379,54 @@ contract VaultRouterBranch {
/// @param shares The amount of index tokens to stake, in 18 decimals.
function stake(uint128 vaultId, uint128 shares) external {
// to prevent safe cast overflow errors
if (shares < Constants.MIN_OF_SHARES_TO_STAKE) {
revert Errors.QuantityOfSharesLessThanTheMinimumAllowed(Constants.MIN_OF_SHARES_TO_STAKE, uint256(shares));
}
// fetch storage slot for vault by id
Vault.Data storage vault = Vault.loadLive(vaultId);
// prepare the `Vault::recalculateVaultsCreditCapacity` call
uint256[] memory vaultsIds = new uint256[](1);
vaultsIds[0] = uint256(vaultId);
// updates the vault's credit capacity and perform all vault state
// transitions before updating `msg.sender` staked shares
Vault.recalculateVaultsCreditCapacity(vaultsIds);
// load distribution data
Distribution.Data storage wethRewardDistribution = vault.wethRewardDistribution;
// cast actor address to bytes32
bytes32 actorId = bytes32(uint256(uint160(msg.sender)));
+ // get the claimable amount of fees
+ UD60x18 amountToClaimX18 = wethRewardDistribution.getActorValueChange(actorId).intoUD60x18();
+
+ // reverts if the claimable amount is NOT 0
+ if (!amountToClaimX18.isZero()) revert Errors.UserHasPendingRewards(actorId, amountToClaimX18.intoUint256());
+
// accumulate the actor's pending reward before staking
wethRewardDistribution.accumulateActor(actorId);
// load actor distribution data
Distribution.Actor storage actor = wethRewardDistribution.actor[actorId];
// calculate actor updated shares amount
UD60x18 updatedActorShares = ud60x18(actor.shares).add(ud60x18(shares));
// update actor staked shares
wethRewardDistribution.setActorShares(actorId, updatedActorShares);
// transfer shares from actor
IERC20(vault.indexToken).safeTransferFrom(msg.sender, address(this), shares);
// emit an event
emit LogStake(vaultId, msg.sender, shares);
}--
///.@notice Initiates a withdrawal request for a given amount of index tokens from the provided vault.
/// @dev Even if the vault doesn't have enough unlocked credit capacity to fulfill the withdrawal request, the
/// user can still initiate it, wait for the withdrawal delay period to elapse, and redeem the shares when
/// liquidity is available.
/// @dev Invariants involved in the call:
