code-423n4 / 2024-04-dyad-findings

8 stars 6 forks source link

Colluding parties manipulate asset balances, inflating TVL, and Kerosine price, leading to distorted decisions and compromised stability. #1014

Closed c4-bot-7 closed 5 months ago

c4-bot-7 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/Vault.kerosine.unbounded.sol#L60-L63 https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/Vault.kerosine.unbounded.sol#L50-L68 https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/Vault.kerosine.unbounded.sol#L60-L64

Vulnerability details

Impact

If multiple parties who own assets in the vaults collude, they can coordinate their actions to manipulate the asset balances and influence the calculated TVL.

#L60-L63

tvl += vault.asset().balanceOf(address(vault)) 
        * vault.assetPrice() * 1e18
        / (10**vault.asset().decimals()) 
        / (10**vault.oracle().decimals());

This line retrieves the asset balance of each vault and uses it to calculate the TVL. If the asset balances are manipulated by colluding parties, it directly impacts the TVL calculation.

By coordinating their actions, such as simultaneously depositing or withdrawing large amounts of assets, the colluding parties can significantly impact the calculated TVL and, consequently, the Kerosine asset price.

Proof of Concept

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/Vault.kerosine.unbounded.sol#L50-L68

Consider a scenario where multiple parties collude to manipulate the asset balances in the Kerosine vaults and exploit the vulnerability in the assetPrice function. https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/Vault.kerosine.unbounded.sol#L60-L64

Step 1: Initial State

Step 2: Colluding Parties' Manipulation

// PartyX deposits into VaultA
VaultA.deposit(2000000);

// PartyY deposits into VaultB
VaultB.deposit(1500000);

// PartyZ deposits into VaultC
VaultC.deposit(1000000);

Step 3: Manipulated Asset Balances

Step 4: Kerosine Price Calculation

uint tvl;
address[] memory vaults = kerosineManager.getVaults();
uint numberOfVaults = vaults.length;
for (uint i = 0; i < numberOfVaults; i++) {
    Vault vault = Vault(vaults[i]);
    tvl += vault.asset().balanceOf(address(vault)) 
            * vault.assetPrice() * 1e18
            / (10**vault.asset().decimals()) 
            / (10**vault.oracle().decimals());
}

Step 5: Manipulated Kerosine Price

uint numerator = tvl - dyad.totalSupply();
uint denominator = kerosineDenominator.denominator();
return numerator * 1e8 / denominator;

Step 6: Impact

Root Cause of Impact:

The root cause of the vulnerability lies in the design of the assetPrice function in the UnboundedKerosineVault contract. The function calculates the Kerosine asset price based on the current asset balances of the Kerosine vaults without considering the possibility of manipulation by colluding parties.

The specific line of code responsible for the vulnerability is: https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/Vault.kerosine.unbounded.sol#L60-L64

tvl += vault.asset().balanceOf(address(vault)) 
        * vault.assetPrice() * 1e18
        / (10**vault.asset().decimals()) 
        / (10**vault.oracle().decimals());

This line retrieves the asset balance of each vault and uses it directly in the TVL calculation. If colluding parties manipulate the asset balances by depositing large amounts of tokens, it artificially inflates the TVL and, consequently, the calculated Kerosine asset price.

Lack of proper safeguards against manipulation and the reliance on the current asset balances make the assetPrice function vulnerable to exploitation by colluding parties.

Tools Used

Manual Review

Recommended Mitigation Steps

Implement time-weighted average price (TWAP) or other price smoothing mechanisms to calculate the Kerosine price based on historical data over a specific time period, making it more resistant to short-term manipulations. And Introduce additional checks and validations to detect and prevent sudden or unusual changes in the asset balances of the vaults.

function assetPrice() public view override returns (uint) {
    uint tvl;
    address[] memory vaults = kerosineManager.getVaults();
    uint numberOfVaults = vaults.length;
    for (uint i = 0; i < numberOfVaults; i++) {
        Vault vault = Vault(vaults[i]);
        uint assetBalance = vault.asset().balanceOf(address(vault));
        uint assetDecimals = vault.asset().decimals();
        uint oracleDecimals = vault.oracle().decimals();
        uint twap = calculateTWAP(address(vault.asset()), assetDecimals, oracleDecimals);
        tvl += assetBalance * twap * 1e18 / (10**assetDecimals) / (10**oracleDecimals);
    }
    uint numerator = tvl - dyad.totalSupply();
    uint denominator = kerosineDenominator.denominator();
    return numerator * 1e8 / denominator;
}

function calculateTWAP(address asset, uint assetDecimals, uint oracleDecimals) internal view returns (uint) {
    // Implement time-weighted average price calculation logic here
    // This can involve fetching historical price data and calculating the average over a specific time period
    // Example implementation:
    uint[] memory prices = fetchHistoricalPrices(asset, oracleDecimals);
    uint twap = 0;
    for (uint i = 0; i < prices.length; i++) {
        twap += prices[i];
    }
    twap /= prices.length;
    return twap;
}

Assessed type

Governance

c4-pre-sort commented 5 months ago

JustDravee marked the issue as duplicate of #67

c4-pre-sort commented 5 months ago

JustDravee marked the issue as sufficient quality report

c4-judge commented 5 months ago

koolexcrypto marked the issue as unsatisfactory: Invalid

c4-judge commented 5 months ago

koolexcrypto marked the issue as satisfactory