Submission Details
Severity:
Vault does not return value change if user stakes more shares
Summary
If user stakes more shares during fee distribution without claiming his fees first, contract will not return him his value change and user will lose his accumulated fees because now his lastValuePerShare will be changed.
Vulnerability Details
In the VaultDistributionBranch.sol:stake() contract does not transfers user's value change if he has some accumulated fees:
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)));
//@audit accumulateActor() function returns valueChange, but stake() function does not transfer it to user
// 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));
//@audit now shares staked with the new valuePerShare and user lost his fees
// 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);
}
Event though Distribution.sol:accumulateActor() returns fee value change:
function accumulateActor(Data storage self, bytes32 actorId) internal returns (SD59x18 valueChange) {
Actor storage actor = self.actor[actorId];
return _updateLastValuePerShare(self, actor, ud60x18(actor.shares));
}
In wethRewardDistribution.accumulateActor(actorId) contract changed user's lastValuePerShare to current valuePerShare and now user unable to claim his fees.
PoC
User stakes his shares in vault, fees distributed, user stakes more shares expecting to also receive his last fees, but contract does not transfer fees to him.
Add this test to test/integration/market-making/fee-distribution-branch/claimFees/claimFees.t.sol:
function testFuzz_WhenFeesUnableToClaimAfterSecondStake(
uint128 vaultId,
uint128 marketId,
uint128 assetsToDeposit,
uint128 marketFees,
uint128 adapterIndex
)
external
{
// ensure valid vault and load vault config
vaultId = uint128(bound(vaultId, INITIAL_VAULT_ID, FINAL_VAULT_ID));
VaultConfig memory fuzzVaultConfig = getFuzzVaultConfig(vaultId);
// all fees paid in WETH
IERC20 wethFeeToken = IERC20(getFuzzVaultConfig(WETH_CORE_VAULT_ID).asset);
// ensure valid deposit amount and perform the deposit
address user = users.naruto.account;
assetsToDeposit =
uint128(bound(assetsToDeposit, calculateMinOfSharesToStake(vaultId) * 2, fuzzVaultConfig.depositCap / 2));
fundUserAndDepositInVault(user, vaultId, assetsToDeposit);
// save and verify pre stake state
ClaimFeesState memory preStakeState =
_getClaimFeesState(user, vaultId, wethFeeToken, IERC20(fuzzVaultConfig.indexToken));
assertGt(preStakeState.stakerVaultBal, 0, "Staker vault balance > 0 after deposit");
// perform the stake
vm.startPrank(user);
marketMakingEngine.stake(vaultId, preStakeState.stakerVaultBal / 2);
// sent market fees from PerpsEngine -> MarketEngine
marketFees = uint128(bound(marketFees, calculateMinOfSharesToStake(vaultId), fuzzVaultConfig.depositCap / 2));
IDexAdapter adapter = getFuzzDexAdapter(adapterIndex);
PerpMarketCreditConfig memory fuzzPerpMarketCreditConfig = getFuzzPerpMarketCreditConfig(marketId);
deal(fuzzVaultConfig.asset, address(fuzzPerpMarketCreditConfig.engine), marketFees);
changePrank({ msgSender: address(fuzzPerpMarketCreditConfig.engine) });
marketMakingEngine.receiveMarketFee(fuzzPerpMarketCreditConfig.marketId, fuzzVaultConfig.asset, marketFees);
assertEq(IERC20(fuzzVaultConfig.asset).balanceOf(address(marketMakingEngine)), marketFees);
// optionally convert asset to WETH if not on WETH vault
if (fuzzVaultConfig.asset != address(wEth)) {
changePrank({ msgSender: address(perpsEngine) });
marketMakingEngine.convertAccumulatedFeesToWeth(
fuzzPerpMarketCreditConfig.marketId, fuzzVaultConfig.asset, adapter.STRATEGY_ID(), bytes("")
);
}
//verify that user has fees to claim
(, int256 valuePerShare, uint128 accountShares, int256 accountLastValuePerShare) = marketMakingEngine.getTotalAndAccountStakingData(vaultId, user);
assert((uint256(accountShares) * uint256(valuePerShare - accountLastValuePerShare)) != 0);
//user stakes more before claiming fees, expecting to receive fees because of value change
vm.startPrank(user);
marketMakingEngine.stake(vaultId, preStakeState.stakerVaultBal / 2);
//user did not received his value fees share
assertEq(IERC20(fuzzVaultConfig.asset).balanceOf(user), 0);
// staker try to claims rewards but cant
changePrank({ msgSender: user });
vm.expectRevert(Errors.NoFeesToClaim.selector);
marketMakingEngine.claimFees(vaultId);
}
Run test in cmd with this command:
forge test --mt testFuzz_WhenFeesUnableToClaimAfterSecondStake
Output:
Ran 1 test for test/integration/market-making/fee-distribution-branch/claimFees/claimFees.t.sol:ClaimFees_Integration_Test
[PASS] testFuzz_WhenFeesUnableToClaimAfterSecondStake(uint128,uint128,uint128,uint128,uint128) (runs: 1007, μ: 5190642, ~: 5218191)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 12.67s (12.63s CPU time)
Impact
Users can't claim their last fees if they added more shares during distribution.
Tools Used
Manual Review
Recommendations
Change stake() function like this:
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)));
// accumulate the actor's pending reward before staking
+ UD60x18 amountToClaimX18 = wethRewardDistribution.accumulateActor(actorId).intoUD60x18();
+
+ if (!amountToClaimX18.isZero()) {
+ // weth address
+ address weth = MarketMakingEngineConfiguration.load().weth;
+ // load the weth collateral data storage pointer
+ Collateral.Data storage wethCollateral = Collateral.load(weth);
+ // convert the amount to claim to weth amount
+ uint256 amountToClaim = wethCollateral.convertUd60x18ToTokenAmount(amountToClaimX18);
+ // transfer the amount to the claimer
+ IERC20(weth).safeTransfer(msg.sender, amountToClaim);
+ }
// 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);
}
Updates
Lead Judging Commences
inallhonesty 4 months ago
Submission Judgement Published Assigned finding tags:
Inside VaultRouterBranch if you stake wait some time then stake again makes you lose the rewards.
Prize pool breakdown
Total prize
70,000 USDC
nSLOC
3,25821 USDC / LOC
66,500
3,500
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.