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

Incorrect vault debt validation logic in rebalanceVaultsAssets causes reverts

elolpuer

Summary

The validation logic in CreditDelegationBranch.sol:rebalanceVaultsAssets() incorrectly reverts when the in-credit vault has a negative unsettledRealizedDebt and the in-debt vault has a positive unsettledRealizedDebt. This is not how function description says, the in-credit vault should have a deficit (< 0), and the in-debt vault should have a surplus (> 0).

Vulnerability Details

CreditDelegationBranch.sol:rebalanceVaultsAssets() function descriptions states that in-credit vault should have negative unsettledRealizedDebt while in-debt vault should have positive unsettledRealizedDebt:

/// @notice Rebalances creditand debt between two vaults.
/// @dev There are multiple factors that may result on vaults backing the same engine having a completely
/// different credit or debt state, such as:
/// - connecting vaults with markets in different times
/// - connecting vaults with different sets of markets
/// - users swapping the engine's usd token for assets of different vaults
/// This way, from time to time, the system keepers must rebalance vaults with a significant state difference in
/// order to facilitate settlement of their credit and debt. A rebalancing doesn't need to always fully settle the
/// amount of USDC that a vault in credit requires to settle its due amount, so the system is optimized to ensure
/// a financial stability of the protocol.
/// @dev Example:
/// in credit vault markets realized debt = -100 -> -90
/// in credit vault deposited usdc = 200 -> 210
/// in credit vault unsettled realized debt = -300 | as -100 + -200 -> after settlement -> -300 | as -90 + -210
/// = -300
​
/// thus, we need to rebalance as the in credit vault doesn't own enough usdc to settle its due credit
​
/// in debt vault markets realized debt = 50 -> 40
/// in debt vault deposited usdc = 10 -> 0
/// in debt vault unsettled realized debt = 40 | as 50 + -10 -> after settlement -> 40 | as 40 + 0 = 40
/// @dev The first vault id passed is assumed to be the in credit vault, and the second vault id is assumed to be
/// the in debt vault.
/// @dev The final unsettled realized debt of both vaults MUST remain the same after the rebalance.
/// @dev The actual increase or decrease in the vaults' unsettled realized debt happen at `settleVaultsDebt`.
/// @param vaultsIds The vaults' identifiers to rebalance.
function rebalanceVaultsAssets(uint128[2] calldata vaultsIds) external onlyRegisteredSystemKeepers {
// load the storage pointers of the vaults in net credit and net debt
Vault.Data storage inCreditVault = Vault.loadExisting(vaultsIds[0]);
Vault.Data storage inDebtVault = Vault.loadExisting(vaultsIds[1]);
​
// both vaults must belong to the same engine in order to have their debt
// state rebalanced, as each usd token's debt is isolated
if (inCreditVault.engine != inDebtVault.engine) {
revert Errors.VaultsConnectedToDifferentEngines();
}
​
// create an in-memory dynamic array in order to call `Vault::recalculateVaultsCreditCapacity`
uint256[] memory vaultsIdsForRecalculation = new uint256[](2);
vaultsIdsForRecalculation[0] = vaultsIds[0];
vaultsIdsForRecalculation[1] = vaultsIds[1];
​
// recalculate the credit capacity of both vaults
Vault.recalculateVaultsCreditCapacity(vaultsIdsForRecalculation);
​
// 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 (
inCreditVaultUnsettledRealizedDebtUsdX18.lte(SD59x18_ZERO)
|| inDebtVaultUnsettledRealizedDebtUsdX18.gte(SD59x18_ZERO)
) {
revert Errors.InvalidVaultDebtSettlementRequest();
}
...
}

But this code block is doing the opposite:

//@AUDIT this is not how requirement should be
//contract should revert if inCreditVaultUnsettledRealizedDebtUsdX18 >= 0
//or inDebtVaultUnsettledRealizedDebtUsdX18 <= 0
if (
inCreditVaultUnsettledRealizedDebtUsdX18.lte(SD59x18_ZERO)
|| inDebtVaultUnsettledRealizedDebtUsdX18.gte(SD59x18_ZERO)
) {
revert Errors.InvalidVaultDebtSettlementRequest();
}

Check how function flow will go with data from function description:

In-credit vault:

realizedDebt = -100
depositedUSDC = 200
unsettledRealizedDebt = (-100) + (-200) = -300

In-debt vault:

realizedDebt = 50
depositedUSDC = 10
unsettledRealizedDebt = 50 + (-10) = 40

Result:

inCreditVaultUnsettledRealizedDebtUsd <= 0 - true
inDebtVaultUnsettledRealizedDebtUsd >= 0 - true
//function will revert

But function should not revert with this parameters, vaults should be rebalanced.

PoC

Add this test to test/integration/market-making/credit-delegation-branch/rebalanceVaultsAssets/rebalanceVaultsAssets.t.sol:

function testFuzz_RevertWhen_InvalidVaultDebtSettlementRequestShouldntBeAsInExample(
uint256 inCreditVaultId,
uint256 inDebtVaultId
)
external
whenTheIndebtVaultAndIncreditVaultsEnginesMatch
{
VaultConfig memory inCreditVaultConfig = getFuzzVaultConfig(inCreditVaultId);
VaultConfig memory inDebtVaultConfig = getFuzzVaultConfig(inDebtVaultId);
​
vm.assume(inCreditVaultConfig.vaultId != inDebtVaultConfig.vaultId);
​
uint128[2] memory vaultIds;
​
vaultIds[0] = inCreditVaultConfig.vaultId;
vaultIds[1] = inDebtVaultConfig.vaultId;
​
changePrank({ msgSender: users.owner.account });
marketMakingEngine.setVaultEngine(inCreditVaultConfig.vaultId, address(1));
marketMakingEngine.setVaultEngine(inDebtVaultConfig.vaultId, address(1));
//using parameters from example
marketMakingEngine.workaround_setVaultDebt(inCreditVaultConfig.vaultId, -100);
marketMakingEngine.workaround_setVaultDepositedUsdc(inCreditVaultConfig.vaultId, 200);
​
marketMakingEngine.workaround_setVaultDebt(inDebtVaultConfig.vaultId, 50);
marketMakingEngine.workaround_setVaultDepositedUsdc(inDebtVaultConfig.vaultId, 10);
​
changePrank({ msgSender: address(perpsEngine) });
​
// by logic it shouldn't revert but it does
vm.expectRevert(abi.encodeWithSelector(Errors.InvalidVaultDebtSettlementRequest.selector));
​
marketMakingEngine.rebalanceVaultsAssets(vaultIds);
}

Run test with command:

forge test --mt testFuzz_RevertWhen_InvalidVaultDebtSettlementRequestShouldntBeAsInExample

Output:

Ran 1 test for test/integration/market-making/credit-delegation-branch/rebalanceVaultsAssets/rebalanceVaultsAssets.t.sol:CreditDelegationBranch_RebalanceVaultsAssets_Integration_Test
[PASS] testFuzz_RevertWhen_InvalidVaultDebtSettlementRequestShouldntBeAsInExample(uint256,uint256) (runs: 1006, μ: 705910, ~: 706406)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.52s (1.49s CPU time)

Impact

CreditDelegationBranch.sol:rebalanceVaultsAssets() does not work as intended and reverts when real in-credit vault needs rebalancing from in-debt vault.

Tools Used

Manual Review

Recommendations

Change requirement:

if (
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:

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.