DYAD - Maroutis's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L64-L65 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L95-L96

Vulnerability details

Impact

Due to a design flaw in the protocol's management of vault licensing and the deploy script, a significant risk exists where collateral can be counted twice in the calculation of the Collateralization Ratio. This occurs because WETH vaults are incorrectly licensed to both the KeroseneManager and VaultLicenser, allowing users to register the same asset and their NFT id in both Kerosene vaults and normal vaults. This can allow users to exploit this to greatly inflate their CR calculations, misleading the protocol into considering a position more secure than it truly is, which can prevent necessary liquidations and pose significant risk to the protocol. Moroever, a user can also register his ID and the keroseneVault as a normal vault because the script calls the licensing function for the kerosineVaults using the VaultLicenser rather than the kerosineManager. This can lead to positions entirely collateralized with kerosene token. Which is not what protocol intends to do and is very risky as the kerosene token is endogenous and has a manipulable asset price.

Proof of Concept

Basically, there are two exploits with devastating impacts on the protocol :

1. The DeployV2 script calls both contract licensing tx functions on the weth vault.

kerosineManager.add(address(ethVault));
kerosineManager.add(address(wstEth));

And

vaultLicenser.add(address(ethVault));
vaultLicenser.add(address(wstEth));

The licensing via the kerosineManager also has to be done, because the weth vaults will be used by the UnboundedKerosineVault contract during the price calculation of the kerosine asset.

This can be exploited where users can call both VaultManagerV2::addKerosene and VaultManagerV2::add functions with their id and the weth vault as parameters. The VaultManagerV2::collatRatio uses both vaults and vaultsKerosene mapping to calculate the value of the stored assets. Since, weth vault is added in both mappings the assets be counted twice.

Consider the following test :

    function test_CanMintSameAmountAsDeposit() public {
        // address RECEIVER2 = makeAddr("Receiver2");

        uint256 id = mintDNft();
        uint256 id2 = mintDNft();
        // Add vault in both contracts
        vaultManagerV2.add(id, address(wethVaultV2));
        vaultManagerV2.add(id2, address(wethVaultV2));
        vaultManagerV2.addKerosene(id, address(wethVaultV2));

        // Deposits 1e25 USD of Weth
        depositV2(weth, id, address(wethVaultV2), 1e22);// Price weth 1000
        // Mint 1e25
        vaultManagerV2.mintDyad(id, 1e25, RECEIVER);

        // Protocol considers that User has deposited twice the amount in the collateral ratio calculation
        console.log("CR of position", vaultManagerV2.collatRatio(id)); // 200%

        // Position is not liquidatable even if it is only collateralized at 100%
        vm.expectRevert(IVaultManager.CrTooHigh.selector);
        vm.prank(RECEIVER);
        vaultManagerV2.liquidate(id, id2);

    }

2. The DeployV2 script calls the licensing tx functions on the kerosine vaults using the vaultLicenser :

        vaultLicenser.add(address(unboundedKerosineVault));
        // vaultLicenser.add(address(boundedKerosineVault));

This has to be done via the vaultLicenser instead of the kerosineManager so that the kerosene vault assets would not be taken into account during the price calculation of the kerosine asset.

A user can just call the VaultManagerV2::add function with their ID and kerosine vault as parameters. Since, the kerosine vault will be stored inside the vaults mappings, some positions can be mostly or entirely collateralized with the kerosine asset, which is not what is intended by the protocol. In fact kerosene is a volatile asset and can cause stability and liquidity issues to the protocol.

Consider the following test :

    function test_addKeroseneAsExoColl() public {
        uint256 id = mintDNft();
        uint256 id2 = mintDNft();

        // Follow script deployement. Weth Vault is licensed in both VaultManager and KerosineManager
        // A user can just add his id and the WethVault in the kerosine mapping and kerosineVault in the vault mapping
        vaultManagerV2.addKerosene(id, address(wethVaultV2));
        vaultManagerV2.add(id, address(unboundedKerosineVault));

        // Assume weth was deposited by other users
        depositV2(weth, id2, address(wethVaultV2), 1e24); //weth 1000 Usd

        // User deposits kerosine using id
        kerosineMock.mint(address(this), 1e20);
        kerosineMock.approve(address(vaultManagerV2), 1e20);
        vaultManagerV2.deposit(id, address(unboundedKerosineVault), 1e20);
        console.log("Kerosine price", unboundedKerosineVault.assetPrice()); //9999

        //Then mint dyad
        vaultManagerV2.mintDyad(id, 1e19, RECEIVER);

        // => Position 150% collateralized with kerosine tokens
        // !! User cannot add kerosine bounded or unbounded vaults in the kerosine mapping in the vault Manager
        // !! and id and weth vault can be added in both kerosene and normal vaults which would make the amount deposited calculated twice in the collateralRatio
    }

Tools Used

Manual review and foundry test

Recommended Mitigation Steps

The design of the licensing part needs to be rethinked. The issue here is that the vaults mapping of the KerosineManager contract which is constructed via the method KerosineManager::add is the same mapping that is used by the UnboundedKerosineVault::assetPrice function. You can consider creating two separate mappings. One used only for the price calculation in the UnboundedKerosineVaultassetPrice contract which would only include the classic vaults (weth ...). And another mapping used for the licensing part which would include the kerosene vaults. Let's assume these were implemented, we have now two mappings. The DeployV2 should change as follow :

        kerosineManager.addVaultForOracleCalculation(address(ethVault));
        kerosineManager.addVaultForOracleCalculation(address(wstEth));

        kerosineManager.add(address(unboundedKerosineVault));
        kerosineManager.add(address(boundedKerosineVault));

Assuming the addVaultForOracleCalculation feeds a mapping that will be used by UnboundedKerosineVault::assetPrice while add doesn't.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L215 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L224-L225

Vulnerability details

Impact

Two similar but different issues are being discussed in this finding. The first one is that the protocol allows users to mint Dyad without any lower limit. Small positions like this can accumulate fast. And the second one is that liquidators will not engage in positions where the usd collateral value is smaller or close the the dyad loan value + any gas costs. The latter can happen frequently since a liquidator cannot chose to liquidate a specific amount but the whole position only. Also, the second issue concerns any position not just smaller ones. Unliquidatable positions like these can quickly accumulate and create a liquidity crisis event.

This makes the impact high and the likelihood of the event medium to high as well.

Proof of Concept

• A position could exist with a very low collateral value. This user is undercollateralized and must be liquidated in order to ensure that the protocol remains overcollateralized. If a liquidator wishes to liquidate this user, they will first need to stake some collateral (weth or other) which involves gas cost. Because the value of the collateral is so low, after gas costs, liquidators will not make a profit. In the end these low value vaults will never get liquidated, leaving the protocol with bad debt. Also malicious users could knowingly creates multiple small positions in order to attack the protocol and depeg the Dyad token.

• The function liquidate expects the liquidator to fully liquidate a position, even if it's not economically viable. The absence of partial liquidation and loan interests could deter liquidators from engaging with less profitable positions. This particular issue regards all sizes small, medium and larges positions. An accumulation of these bad debts can actually trigger a liquidity crisis.

In both of these cases, the rule is simple, liquidators will only liquidate a position if :

vaultsAssets * liquidationAssetShare > mintedDyadLoan + Gas costs

By default any position with a CR=100% or less will not be liquidated.

Not having the flexibity of partially liquidating a position so that a non profitable position becomes profitable increases the risk of bad debt. Similarly, not having any interest payments also participates in the lack of incentives. As increasing interests worsens CR and increases incentives.

Tools Used

Manual review

Recommended Mitigation Steps

Consider implementing the following changes to significantly minimize bad debt :

• Introduce partial liquidations
• Introduce interest payements that increase as the time passes
• Set a minimum threshold for collateral value which has to be exceeded in order for a user to mint Dyad
• For the bad debt that subsides even after all these changes, implement an auction system that would sell the collateral to remove any lingering bad debts.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L213-L214

Vulnerability details

Impact

The liquidation function only checks if the collateral ratio (CR) is greater than 150%, without validating if the non-Kerosene value remains at or above 100%. This oversight could allow scenarios where excessive endogenous (Kerosene) >50% collateralization is used.

This goes against the protocol system as both mintDyad and withdraw functions have this check. Secondly, having more kerosene collateral is more risky for the protocol as the kerosene asset can be considered slightly endougenous due to it's dependence to Dyad token in the calculation of it's asset price and the rewarding system. Lastly, a liquidator can find himself stuck and not being able to withdraw or mint because the kerosene tokens makes up more than 50% of his collateral if he liquidates a position that has a significant kerosene size.

Proof of Concept

The liquidate function calculates and checks the collateral ratio using the total value:

        uint256 cr = collatRatio(id);
        if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();

In both mintDyad and withdraw functions, the exogenous collateral is checked to be 100%. if (getNonKeroseneValue(id) < newDyadMinted) revert NotEnoughExoCollat();

And

if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();

This is also mentioned in the docs :

These equations ensure that Notes can only withdraw non-Kerosene collateral and/ or mint more DYAD if there will be at least $1 of non-Kerosene collateral per DYAD minted in their Note upon the successful execution of the mint or withdrawal transaction.

Not having a check can break this invariant. A position can have a healthy CR (>=150% collateralized) but an exogenous collateral less than 100% of the debt. Having a signficant portion of kerosene tokens as collateral >50% can be problematic due to its dependence to the Dyad token and especially in case where the asset price of the kerosene token is manipulated.

Tools Used

Manual review

Recommended Mitigation Steps

Consider adapting the check at the top of the liquidate function as follow :

        uint256 cr = collatRatio(id);
        if (cr >= MIN_COLLATERIZATION_RATIO && getNonKeroseneValue(id) >= dyad.mintedDyad(address(this), id)) revert CrTooHigh();

This will make the position liquidatable if exogenous collateral is less than 100% or in case where cr < 150%.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Vault.kerosine.unbounded.sol#L65

Vulnerability details

Impact

The UnboundedKerosineVault::assetPrice() function calculates the price of Kerosine based TVL in the protocol's vaults and the total supply of Dyad. An underflow can occur if the TVL falls below the total Dyad supply, resulting in a negative numerator in the price calculation. This scenario is particularly likely if the price of collateral assets such as WETH decreases sharply, or if Kerosine itself is used as collateral in significant amounts. Such an underflow prevents withdrawals of assets, the minting of new Dyad tokens and the liquidation of undercollateralized positions, which effectively traps users' assets.

Proof of Concept

Many scenarios can cause a sudden fall in TVL :

1. The price of collateral falls. Since the protocol forces users to have 100% exogenous collateral. TVL can sharply decrease as oracle price falls.
2. Big depositors withdraw their deposits due to market volatility.
3. Since the kerosene vaults can be added directly into the vaults mappings (rather than kerosine vaults). Some positions will be 100% collateralized by kerosine.

The following test scenario illustrates how an underflow can occur in the UnboundedKerosineVault::assetPrice() function when oracle price of weth asset falls:

    function test_KerosineAssetPriceUnderflow() public {
        uint256 id = mintDNft();
        uint256 id2 = mintDNft();

        // Follow script deployement. Weth Vault is licensed in both VaultManager and KerosineManager
        vaultManagerV2.add(id, address(unboundedKerosineVault));
        vaultManagerV2.add(id, address(wethVaultV2));

        // User deposits weth
        depositV2(weth, id, address(wethVaultV2), 1e22); //weth 1000 Usd

        // User deposits kerosine using id
        kerosineMock.mint(address(this), 1e20);
        kerosineMock.approve(address(vaultManagerV2), 1e20);
        vaultManagerV2.deposit(id, address(unboundedKerosineVault), 1e20);
        console.log("Kerosine price", unboundedKerosineVault.assetPrice()); //9999

        //Then mint dyad
        vaultManagerV2.mintDyad(id, 1e24, RECEIVER);
        // Assume price decreases
        wethOracle.setPrice(99e8);

        // We now have tvl < mintedDyad. Many reasons can cause this.
        // 1. The protocol forces the users to have 100% in exo only. If price falls then so the tvl
        // 2. since kerosene vaults can be added directly into the vaults mappings (rather than kerosine vaults). Some positions will be 100% collateralized by kerosine.
        // Position cannot be liquidated nor can the user withdraw his collateral.
        // This can happen even if position is not liquidatable.
        vm.expectRevert(stdError.arithmeticError);
        vm.prank(RECEIVER);
        vaultManagerV2.liquidate(id, id2);
    }

In this scenario, the drop in WETH price leads to a TVL that is less than the Dyad issued, causing an arithmetic underflow when calculating the Kerosine price, which in turn prevents liquidation, withdrawals and mints.

Tools Used

Manual review and foundry test suite

Recommended Mitigation Steps

• Consider introducing checks in the UnboundedKerosineVault::assetPrice() function to ensure that the numerator cannot go negative. Implementing a minimum floor for the asset price can prevent underflows.

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L221-L226

Vulnerability details

Impact

A liquidatable position is determined by the collatRatio which is calculated using both vaults assets and kerosineVaults assets. A liquidator determines if a liquidation is profitable for him by calculating the collatRatio determined using the whole USD value of a position. However, the liquidation function does not transfer Kerosene-based vault assets to the liquidator, leading to potential losses for liquidators and diminishing the incentives of performing liquidations.

Proof of Concept

First, the VaultManagerV2::liquidate function determines if a position is liquidatable :

uint256 cr = collatRatio(id);
if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();

What's important to note here is that the collatRatio is calculated using the getTotalUsdValue which is the sum of kerosene and nonKerosene assets getNonKeroseneValue(id) + getKeroseneValue(id). getNonKeroseneValue function uses the vaults mapping while getKeroseneValue uses the vaultsKerosene.

However, when calculating the amount to be awarded to the liquidator, only a portion (or the whole position depending on the cr) of the vaults mapping assets, thus the non kerosine assets are transfered:

        uint256 numberOfVaults = vaults[id].length();
        for (uint256 i = 0; i < numberOfVaults; i++) {
            Vault vault = Vault(vaults[id].at(i));
            uint256 collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare);
            vault.move(id, to, collateral);
        }

According to the implemention and sponsors, users can push down the explicit exogenous CR to 100% : if (getNonKeroseneValue(id) < newDyadMinted) revert NotEnoughExoCollat();

And cover the rest with Kerosene : if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow();

This means that for a liquidatable position with a CR of 1.40. Assuming for simplicity that kerosene assets make 0.5 of CR meaning exogenous assets make the rest i.e 0.9. Liquidator would expect 77% of the whole collateral position as reward which would give him an additional of 8% additional value compared to the burned dyad. For details: whole_position * 77% - burned_Dyad = burned_Dyad (1+0.08). Since whole_position = burned_Dyad * 1.40.

However, he will only get 77% of the non kerosine value, i.e 77% of the 0.9 assets that makes the CR. This means that he will burn his dyad but only get an equivalent value of 77%*0.9*(value of the burned dyad) which is < burned_Dyad. Thus losing value.

Tools Used

Manual review and foundry testing suite.

Recommended Mitigation Steps

The protocol needs to move the kerosene assets to the liquidators as well to make sure that value is not lost when liquidating.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L215 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L224-L225

Vulnerability details

Impact

Two similar but different issues are being discussed in this finding. The first one is that the protocol allows users to mint Dyad without any lower limit. Small positions like this can accumulate fast. And the second one is that liquidators will not engage in positions where the usd collateral value is smaller or close the the dyad loan value + any gas costs. The latter can happen frequently since a liquidator cannot chose to liquidate a specific amount but the whole position only. Also, the second issue concerns any position not just smaller ones. Unliquidatable positions like these can quickly accumulate and create a liquidity crisis event.

This makes the impact high and the likelihood of the event medium to high as well.

Proof of Concept

• A position could exist with a very low collateral value. This user is undercollateralized and must be liquidated in order to ensure that the protocol remains overcollateralized. If a liquidator wishes to liquidate this user, they will first need to stake some collateral (weth or other) which involves gas cost. Because the value of the collateral is so low, after gas costs, liquidators will not make a profit. In the end these low value vaults will never get liquidated, leaving the protocol with bad debt. Also malicious users could knowingly creates multiple small positions in order to attack the protocol and depeg the Dyad token.

• The function liquidate expects the liquidator to fully liquidate a position, even if it's not economically viable. The absence of partial liquidation and loan interests could deter liquidators from engaging with less profitable positions. This particular issue regards all sizes small, medium and larges positions. An accumulation of these bad debts can actually trigger a liquidity crisis.

In both of these cases, the rule is simple, liquidators will only liquidate a position if :

vaultsAssets * liquidationAssetShare > mintedDyadLoan + Gas costs

By default any position with a CR=100% or less will not be liquidated.

Not having the flexibity of partially liquidating a position so that a non profitable position becomes profitable increases the risk of bad debt. Similarly, not having any interest payments also participates in the lack of incentives. As increasing interests worsens CR and increases incentives.

Tools Used

Manual review

Recommended Mitigation Steps

Consider implementing the following changes to significantly minimize bad debt :

• Introduce partial liquidations
• Introduce interest payements that increase as the time passes
• Set a minimum threshold for collateral value which has to be exceeded in order for a user to mint Dyad
• For the bad debt that subsides even after all these changes, implement an auction system that would sell the collateral to remove any lingering bad debts.

Assessed type

Other