Summary
Because of an arithmetic underflow in the recalculateVaultsCreditCapacitythe vault is totally DOSed after a redemption.
Vulnerability Details
For every change in the state of a vault, the function recalculateVaultsCreditCapacityVault:369 is called. The purpose of this function is to recalculate the latest credit capacity of the provided vault IDs, taking into account their most recent asset and debt values, denominated in USD.
The weight of all credit delegations is set to the current vault assets, as we can see here (Vault:521):
uint128 newWeight = uint128(IERC4626(self.indexToken).totalAssets());
for (uint256 i; i < connectedMarketsIdsCache.length; i++) {
CreditDelegation.Data storage creditDelegation =
CreditDelegation.load(self.id, connectedMarkets.at(i).toUint128());
creditDelegation.weight = newWeight;
}
The function calls _updateCreditDelegations Vault:584 to update the credit delegations of the vault and compute the new totalDelegatedCreditUsd of the market, as well as the new valueUSD of the credit delegation, as illustrated here:
uint128 totalCreditDelegationWeightCache = self.totalCreditDelegationWeight;
if (totalCreditDelegationWeightCache != 0) {
UD60x18 creditDelegationShareX18 =
ud60x18(creditDelegation.weight).div(ud60x18(totalCreditDelegationWeightCache));
vaultCreditCapacityUsdX18 = getTotalCreditCapacityUsd(self);
UD60x18 newCreditDelegationUsdX18 = vaultCreditCapacityUsdX18.gt(SD59x18_ZERO)
? vaultCreditCapacityUsdX18.intoUD60x18().mul(creditDelegationShareX18)
: UD60x18_ZERO;
UD60x18 creditDeltaUsdX18 = newCreditDelegationUsdX18.sub(previousCreditDelegationUsdX18);
Market.Data storage market = Market.load(connectedMarketId);
market.updateTotalDelegatedCredit(creditDeltaUsdX18);
if (newCreditDelegationUsdX18.isZero()) {
creditDelegation.clear();
} else {
creditDelegation.valueUsd = newCreditDelegationUsdX18.intoUint128();
}
}
}
The problem occurs in the calculation of creditDeltaUsdX18. If the system generates more debt or if the amount of assets decreases, the newCreditDelegationUsdX18 will be lower than the previousCreditDelegationUsdX18. As a result, the call will revert after a redeem or any other function that modifies the state of the vault and decrease the credit delegation value.
POC
YYou can run the POC by copying and pasting this code into the redeem.t.sol file and executing the following command: forge test --mt test_UnderflowPOC -vvv
You should have this output :
Ran 1 test suite in 768.95ms (44.09ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)
Failing tests:
Encountered 1 failing test in test/integration/market-making/vault-router-branch/redeem/redeem.t.sol:Redeem_Integration_Test
[FAIL: panic: arithmetic underflow or overflow (0x11)] test_UnderflowPOC() (gas: 1788080)
function test_UnderflowPOC() public {
vm.stopPrank();
uint128 vaultId= INITIAL_VAULT_ID;
uint128 assetsToDeposit= uint128(calculateMinOfSharesToStake(vaultId));
uint128 sharesToWithdraw;
VaultConfig memory fuzzVaultConfig = getFuzzVaultConfig(vaultId);
address user = users.naruto.account;
fundUserAndDepositInVault(user, vaultId, assetsToDeposit);
uint128 userVaultShares = uint128(IERC20(fuzzVaultConfig.indexToken).balanceOf(user));
sharesToWithdraw = userVaultShares/2;
vm.startPrank(user);
marketMakingEngine.initiateWithdrawal(vaultId, sharesToWithdraw);
skip(fuzzVaultConfig.withdrawalDelay + 1);
UD60x18 expectedAssetsX18 = marketMakingEngine.getIndexTokenSwapRate(vaultId, sharesToWithdraw, false);
uint256 redeemFee = vaultsConfig[vaultId].redeemFee;
UD60x18 expectedAssetsMinusRedeemFeeX18 = expectedAssetsX18.sub(expectedAssetsX18.mul(ud60x18(redeemFee)));
UD60x18 sharesMinusRedeemFeesX18 =
marketMakingEngine.getVaultAssetSwapRate(vaultId, expectedAssetsMinusRedeemFeeX18.intoUint256(), false);
RedeemState memory pre = _getRedeemState(
user, users.vaultFeeRecipient.account, IERC20(fuzzVaultConfig.asset), IERC20(fuzzVaultConfig.indexToken)
);
assertEq(pre.marketEngineVaultBal, sharesToWithdraw, "MarketEngine received shares from initiated withdraw");
assertEq(pre.redeemerVaultBal, userVaultShares - sharesToWithdraw, "Shares deducted from Redeemer");
assertEq(pre.feeReceiverAssetBal + pre.vaultAssetBal, assetsToDeposit, "All deposited assets accounted");
vm.expectEmit();
emit VaultRouterBranch.LogRedeem(vaultId, user, sharesMinusRedeemFeesX18.intoUint256());
marketMakingEngine.redeem(vaultId, WITHDRAW_REQUEST_ID, 0);
RedeemState memory post = _getRedeemState(
user, users.vaultFeeRecipient.account, IERC20(fuzzVaultConfig.asset), IERC20(fuzzVaultConfig.indexToken)
);
WithdrawalRequest.Data memory withdrawalRequest =
marketMakingEngine.exposed_WithdrawalRequest_loadExisting(vaultId, user, WITHDRAW_REQUEST_ID);
assertTrue(withdrawalRequest.fulfilled);
assertEq(
post.feeReceiverAssetBal - pre.feeReceiverAssetBal,
expectedAssetsX18.mul(ud60x18(redeemFee)).intoUint256(),
"Redeem fees paid to FeeReceiver"
);
assertEq(post.redeemerVaultBal, userVaultShares - sharesToWithdraw, "Shares deducted from Redeemer");
assertEq(post.marketEngineVaultBal, 0, "No shares stuck in market engine");
assertEq(
post.redeemerAssetBal,
expectedAssetsMinusRedeemFeeX18.intoUint256(),
"Redeemer received correct asset tokens"
);
assertEq(
post.redeemerAssetBal + post.feeReceiverAssetBal + post.vaultAssetBal,
assetsToDeposit,
"All deposited assets accounted"
);
marketMakingEngine.initiateWithdrawal(vaultId, sharesToWithdraw);
skip(fuzzVaultConfig.withdrawalDelay + 1);
marketMakingEngine.redeem(vaultId, WITHDRAW_REQUEST_ID +1, 0);
}
Impact
Since this function is called every time the state of the vault is modified, the vault is completely DOSed.
Tools Used
Echidna
Recommendations
The protocol should change the type of the creditDeltaUsdX18 in order to be negative :
SD59x18 previousCreditDelegationUsdX18 = ud60x18(creditDelegation.valueUsd).intoSD59x18();
...
SD59x18 creditDeltaUsdX18 = newCreditDelegationUsdX18.intoSD59x18().sub(previousCreditDelegationUsdX18);
