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

Reverse logic in `settleVaultsDebt` allows usdToken holders to steal Vault assets

alexczm

Summary

CreditDelegationBranch::settleVaultsDebt() function uses incorrect comparison and causes assets to be swapped in the wrong direction when settling vault debt/credit positions. This allows users to swap usdTokens for vault assets at a better ratio.

Vulnerability Details

Note: this submission has 3 parts. First and third part describe the same problem but in different parts of the codebase. The second part builds on top of part 1, thus keeping them togheter.

Part 1

vault.getUnsettledRealizedDebt() returns an SD59x18 type value representing the vault's debt/credit position. A negative value indicates a credit balance, meaning the vault has accumulated USDC that should be swapped to the vault's asset. A positive value indicates debt, requiring the vault's assets to be swapped to USDC for coverage. This is the core purpose of the settleVaultsDebt function.

The function first queries getUnsettledRealizedDebt() to determine the debtOrCredit value, then checks whether the vault is in debt or credit. However, the logic for this determination is inverted: when vaultUnsettledRealizedDebtUsdX18 is less than 0, the vault is actually in credit, not debt as the current code assumes.

function settleVaultsDebt(uint256[] calldata vaultsIds) external onlyRegisteredSystemKeepers {
...
for (uint256 i; i < vaultsIds.length; i++) {
...
​
// cache the vault's unsettled debt, if zero skip to next vault
// amount in zaros internal precision
ctx.vaultUnsettledRealizedDebtUsdX18 = vault.getUnsettledRealizedDebt();
if (ctx.vaultUnsettledRealizedDebtUsdX18.isZero()) continue;
​
...
// if the vault is in debt, swap its assets to USDC
@> if (ctx.vaultUnsettledRealizedDebtUsdX18.lt(SD59x18_ZERO)) { // @audit the logic is inversed; if realizedDebt < 0 then vault is in credit
} else { // if realizedDebt > 0, vault is in debt: swap asset to USDC
// else vault is in credit, swap its USDC previously accumulated
// from market and vault deposits into its underlying asset
...
}

Part 2

When vault is in credit and vault's assets are swapped to USDC vault.marketsRealizedDebtUsd is decreased by the amount of USDC it received, decreasing realized debt.

// deduct the amount of usdc swapped for assets from the vault's unsettled debt
vault.marketsRealizedDebtUsd -= ctx.usdcOutX18.intoUint256().toInt256().toInt128();

This variable is used in getUnsettledRealizedDebt meaning that vault's total unsettled debt will be smaller. A value smaller than 0 representing a credit for the vault.

function getUnsettledRealizedDebt(Data storage self)
internal
view
returns (SD59x18 unsettledRealizedDebtUsdX18)
{
unsettledRealizedDebtUsdX18 =
sd59x18(self.marketsRealizedDebtUsd).add(unary(ud60x18(self.depositedUsdc).intoSD59x18()));
}

getUnsettledRealizedDebt is used to calculate Vault::getTotalDebt, which is called in StabilityBranch.getAmountOfAssetOut to calculate the premiumDiscountFactor .

Because the total debt is reduced (negative) and vault.totalAssets is smaller, the premiumDiscountFactor will be artificially inflated. The amountOut returned by getAmountOfAssetOut to be artificially inflated.

function getAmountOfAssetOut(
uint128 vaultId,
UD60x18 usdAmountInX18,
UD60x18 indexPriceX18
)
public
view
returns (UD60x18 amountOutX18)
{
// fetch the vault's storage pointer
Vault.Data storage vault = Vault.load(vaultId);
​
// fetch the vault's total assets in USD; if the vault is empty
// revert here to prevent panic from subsequent divide by zero
// @audit vault holds less assets after `settleVaultsDebt`
@> UD60x18 vaultAssetsUsdX18 = ud60x18(IERC4626(vault.indexToken).totalAssets()).mul(indexPriceX18);
if (vaultAssetsUsdX18.isZero()) revert Errors.InsufficientVaultBalance(vaultId, 0, 0);
​
// we use the vault's net sum of all debt types coming from its connected markets to determine the swap rate
@> SD59x18 vaultDebtUsdX18 = vault.getTotalDebt();//@audit totalDebt < 0 after calling `settleVaultsDebt`
​
// calculate the premium or discount that may be applied to the vault asset's index price
// note: if no premium or discount needs to be applied, the premiumDiscountFactorX18 will be
// 1e18 (UD60x18 one value)
UD60x18 premiumDiscountFactorX18 =
@> UsdTokenSwapConfig.load().getPremiumDiscountFactor(vaultAssetsUsdX18, vaultDebtUsdX18);
​
// get amounts out taking into consideration the CL price and the premium/discount
amountOutX18 = usdAmountInX18.div(indexPriceX18).mul(premiumDiscountFactorX18);
}

By calling StabilityBranch::initiateSwap and StabilityBranch::fulfillSwap users can swap usdToken to vault's asset and receive more assets than they should.

Note: There's an issue in getPremiumDiscountFactor reported in a separate submission that makes this function to return a wrong factor multiplier.

Calling settleVaultsDebt will swap vault's assets to USDC when the vault is in profit. This allows users to swap usdToken for vault's asset with a premium.

Part 3

Another instance of the same issue described in first part is in rebalanceVaultAssets

function rebalanceVaultsAssets(uint128[2] calldata vaultsIds) external onlyRegisteredSystemKeepers {
...
// cache the in debt vault & in credit vault unsettled debt
SD59x18 inDebtVaultUnsettledRealizedDebtUsdX18 = inDebtVault.getUnsettledRealizedDebt();
SD59x18 inCreditVaultUnsettledRealizedDebtUsdX18 = inCreditVault.getUnsettledRealizedDebt();
​
​
// revert if 1) the vault that is supposed to be in credit is not OR
// 2) the vault that is supposed to be in debt is not
if ( // @audit inversed logic
@> inCreditVaultUnsettledRealizedDebtUsdX18.lte(SD59x18_ZERO)
@> || inDebtVaultUnsettledRealizedDebtUsdX18.gte(SD59x18_ZERO)
) {
revert Errors.InvalidVaultDebtSettlementRequest();
}
...

Impact

Protocol features doesn't work as intended, allowing usdToken holders to steal funds from the vault.

Tools Used

Recommendations

Update settleVaultsDebt and rebalanceVaultsAssets function accordingly:

function settleVaultsDebt(uint256[] calldata vaultsIds) external onlyRegisteredSystemKeepers {
...
- if (ctx.vaultUnsettledRealizedDebtUsdX18.lt(SD59x18_ZERO)) {
+ if (ctx.vaultUnsettledRealizedDebtUsdX18.gt(SD59x18_ZERO)) {
function rebalanceVaultsAssets(uint128[2] calldata vaultsIds) external onlyRegisteredSystemKeepers {
...
if (
- inCreditVaultUnsettledRealizedDebtUsdX18.lte(SD59x18_ZERO)
- || inDebtVaultUnsettledRealizedDebtUsdX18.gte(SD59x18_ZERO)
+ inCreditVaultUnsettledRealizedDebtUsdX18.gte(SD59x18_ZERO)
+ || inDebtVaultUnsettledRealizedDebtUsdX18.lte(SD59x18_ZERO)
) {
revert Errors.InvalidVaultDebtSettlementRequest();
}
...
Updates

Lead Judging Commences

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

settleVaultDebt functions in opposite direction because of `ctx.vaultUnsettledRealizedDebtUsdX18.lt(SD59x18_ZERO)` having an inversed comparator (should have been gt)

Improper if conditions inside `rebalanceVaultsAssets` cause reverts when it shouldn't

Support

FAQs

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