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

`FeeDistributionBranch._handleWethRewardDistribution` double counts fees.

qpzm

Summary

FeeDistributionBranch._handleWethRewardDistribution distributes the fee of a market by incrementing the valuePerShare of all connected vaults.
If one market received $1 fee, all vaults connected to the market increments its valuePerShare as if they received $1.

Vulnerability Details

  1. Market.receiveWethReward adds the all receivedVaultsWethRewardX18 to self.wethRewardPerVaultShare.

self.wethRewardPerVaultShare =
ud60x18(self.wethRewardPerVaultShare).add(receivedVaultsWethRewardX18).intoUint128();

https://github.com/Cyfrin/2025-01-zaros-part-2/blob/fc64c07f42ca7811aa4a8929b206e0c936a8b373/src/market-making/leaves/Market.sol#L524-L525

  1. FeeDistributionBranch._handleWethRewardDistribution calls Vault.recalculateVaultsCreditCapacity to update
    Distribution.Data.valuePerShare of all connected vaults.

Vault.recalculateVaultsCreditCapacity(market.getConnectedVaultsIds());

https://github.com/Cyfrin/2025-01-zaros-part-2/blob/fc64c07f42ca7811aa4a8929b206e0c936a8b373/src/market-making/branches/FeeDistributionBranch.sol#L395

  1. Vault.recalculateVaultsCreditCapacity calls _recalculateConnectedMarketsState to calculate the vault's total
    WETH reward change. It is in the for loop, so every vault calls _recalculateConnectedMarketsState.

for (uint256 i; i < vaultsIds.length; i++) {
// ...
UD60x18 vaultTotalWethRewardChangeX18
) = _recalculateConnectedMarketsState(self, connectedMarketsIdsCache, true);

https://github.com/Cyfrin/2025-01-zaros-part-2/blob/fc64c07f42ca7811aa4a8929b206e0c936a8b373/src/market-making/leaves/Vault.sol#L398-L399

  1. _recalculateConnectedMarketsState calls market.getVaultAccumulatedValues for all connected markets.
    https://github.com/Cyfrin/2025-01-zaros-part-2/blob/fc64c07f42ca7811aa4a8929b206e0c936a8b373/src/market-making/leaves/Vault.sol#L327

  2. Market.getVaultAccumulatedValues calculates the difference between wethRewardPerVaultShare of the market and
    lastVaultDistributedWethRewardPerShareX18 of the vault. The problem is that wethRewardPerVaultShare is
    increased by receivedVaultsWethRewardX18, namely the whole amount of fee in step1.

wethRewardChangeX18 = ud60x18(self.wethRewardPerVaultShare).sub(lastVaultDistributedWethRewardPerShareX18);

https://github.com/Cyfrin/2025-01-zaros-part-2/blob/fc64c07f42ca7811aa4a8929b206e0c936a8b373/src/market-making/leaves/Market.sol#L321

  1. _recalculateConnectedMarketsState updates Distribution.Data.valuePerShare
    of the vault through self.wethRewardDistribution.distributeValue.

  2. The for loop in Vault.recalculateVaultsCreditCapacity mentioned in step 2, continues and updates all the other
    vaults with the same wethRewardPerVaultShare.
    This double counts the fee for vaults.

POC

In the test settings, every market is connected to all vaults. By calling receiveMarketFee for ETH_USD_MARKET_ID,
the fee is distributed to all vaults. However, the fee is double counted for each vault.
https://github.com/Cyfrin/2025-01-zaros-part-2/blob/fc64c07f42ca7811aa4a8929b206e0c936a8b373/test/Base.t.sol#L581'

Copy testPOC_receiveMarketFee_doubleCounts test function at the end of
unstake.t.sol.

Then run forge test --mt testPOC_receiveMarketFee_doubleCounts.

contract Unstake_Integration_Test is Base_Test {
// ...
function testPOC_receiveMarketFee_doubleCounts() external {
// ensure valid vault and load vault config
uint128 vaultId = WETH_CORE_VAULT_ID;
VaultConfig memory fuzzVaultConfig = getFuzzVaultConfig(vaultId);
​
// ensure valid deposit amount and perform the deposit
console.log("---user1 deposit---");
address user = users.naruto.account;
uint128 assetsToDeposit = 1e18;
console.log("assetsToDeposit: %s", assetsToDeposit);
fundUserAndDepositInVault(user, vaultId, assetsToDeposit);
​
// save and verify pre state
UnstakeState memory preStakeState =
_getUnstakeState(user, vaultId, IERC20(fuzzVaultConfig.asset), IERC20(fuzzVaultConfig.indexToken));
assertGt(preStakeState.stakerVaultBal, 0, "Staker vault balance > 0 after deposit");
​
console.log("---user1 stake---");
vm.startPrank(user);
marketMakingEngine.stake(vaultId, preStakeState.stakerVaultBal);
vm.stopPrank();
​
uint128 vaultId2 = vaultId + 1;
VaultConfig memory fuzzVaultConfig2 = getFuzzVaultConfig(vaultId2);
​
console.log("---user1 deposit to another vault ---");
user = users.naruto.account;
assetsToDeposit = uint128(calculateMinOfSharesToStake(vaultId2));
console.log("assetsToDeposit: %s", assetsToDeposit);
fundUserAndDepositInVault(user, vaultId2, assetsToDeposit);
​
// save and verify pre state
preStakeState = _getUnstakeState(user, vaultId2, IERC20(fuzzVaultConfig2.asset), IERC20(fuzzVaultConfig2.indexToken));
assertGt(preStakeState.stakerVaultBal, 0, "Staker vault balance > 0 after deposit");
​
console.log("---user1 stake to another vault ---");
vm.startPrank(user);
marketMakingEngine.stake(vaultId2, preStakeState.stakerVaultBal);
vm.stopPrank();
​
// sent WETH market fees from PerpsEngine -> MarketEngine
uint256 marketFees = 1e18;
deal(fuzzVaultConfig.asset, address(perpMarketsCreditConfig[ETH_USD_MARKET_ID].engine), marketFees);
vm.startPrank(address(perpMarketsCreditConfig[ETH_USD_MARKET_ID].engine));
vm.expectEmit({ emitter: address(marketMakingEngine) });
emit FeeDistributionBranch.LogReceiveMarketFee(fuzzVaultConfig.asset, ETH_USD_MARKET_ID, marketFees);
​
assertEq(marketMakingEngine.workaround_Vault_getValuePerShare(vaultId), 0);
assertEq(marketMakingEngine.workaround_Vault_getValuePerShare(vaultId + 1), 0);
​
uint256 earnedFeesBefore = marketMakingEngine.getEarnedFees(vaultId, user);
console.log("earnedFeesBefore: %s", earnedFeesBefore);
​
console.log("---receiveMarketFee---");
marketMakingEngine.receiveMarketFee(ETH_USD_MARKET_ID, fuzzVaultConfig.asset, marketFees);
vm.stopPrank();
assertEq(IERC20(fuzzVaultConfig.asset).balanceOf(address(marketMakingEngine)), marketFees);
​
// @audit one market received a fee, but all vaults whose id is bigger than `vaultId` received the same fee.
console.log("getValuePerShare");
int256 vaultValuePerShare1 = marketMakingEngine.workaround_Vault_getValuePerShare(vaultId - 1);
assertEq(vaultValuePerShare1, 0);
​
int256 vaultValuePerShare = marketMakingEngine.workaround_Vault_getValuePerShare(vaultId);
assertEq(vaultValuePerShare, 909090909090909090);
​
int256 vaultValuePerShare2 = marketMakingEngine.workaround_Vault_getValuePerShare(vaultId + 1);
assertEq(vaultValuePerShare2, 909090909090909090);
​
uint256 earnedFees = marketMakingEngine.getEarnedFees(vaultId, user);
uint256 earnedFeesFromVault2 = marketMakingEngine.getEarnedFees(vaultId2, user);
assertEq(earnedFees, 0.899999999999999999e18);
assertEq(earnedFeesFromVault2, 0.899999999999999999e18);
​
vm.startPrank(user);
marketMakingEngine.claimFees(vaultId);
// @audit
// The vault has no balance to distribute because the fee is double counted.
// revert with ERC20InsufficientBalance(0xe98A0C3C4dE1ea937C993928a5AED70f9ba593ba, 100000000000000001 [1e17], 899999999999999999 [8.999e17])
marketMakingEngine.claimFees(vaultId2);
vm.stopPrank();
}
}

Impact

These functions in FeeDistributionBranch uses _handleWethRewardDistribution, so double count fees.

  1. receiveMarketFee

  2. convertAccumulatedFeesToWeth

Tools Used

Foundry.

Recommendations

Market.receiveWethReward must increase receivedVaultsWethRewardX18 not by the all fee but by the fee per vault
share.

Updates

Lead Judging Commences

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

`wethRewardPerVaultShare` is incremented by `receivedVaultWethReward` amount which is not divided by number of shares.

Support

FAQs

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

Give us feedback!