DYAD - carrotsmuggler's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/44becc2f09c3a75bd548d5ec756a8e88a345e826/src/core/VaultManagerV2.sol#L119-L131

Vulnerability details

Impact

The function deposit has the modifier isValidDNft(id), thus allowing any user to deposit tokens to any other user's account, since there is no ownership check. The only check is on validity, which makes sure that the id in question exists.

modifier isValidDNft(uint id) {
    if (dNft.ownerOf(id) == address(0))   revert InvalidDNft(); _;
  }

The issue with this is that the protocol implements a flashloan resistance mechanism. It tracks a value idToBlockOfLastDeposit[id] which is the block number of the last deposit, and forbids withdrawals if the same block number is present.

// function deposit:
idToBlockOfLastDeposit[id] = block.number;

// function withdraw:
function withdraw(
    uint    id,
    address vault,
    uint    amount,
    address to
  )
    public
      isDNftOwner(id)
  {
    if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();

Thus if someone deposits even a single wei of any token to a user's account, they cannot make a withdrawal that block.

This can be used to grief users. User's trying to withdraw their funds can be frontrun with a single wei deposit, and they will not be able to withdraw their funds.

This is actually beneficial to the attacker due to the way kerosene works. Kerosene's value is determined by the free collateral in the system divided by the amount of kerosene in supply.

K_val = (TVL - dyad_supply)/K_supply

When a user makes a withdrawal, they reduce the TVL, reducing the value of kerosene tokens. Thus this griefing attack can be used by attackers to prevent withdrawals, thus keeping the value of kerosene tokens propped up artificially. This can cause damage to the system since kerosene tokens make up some of the backing in the system, keeping positions above the minimum collateral ratio.

Proof of Concept

Lets say the attacker has a large position in the system, close to a CR of 150%. Lets say the value of kerosene token is 100 USDC.

1. Alice has an account with 0 debt, and 1 million USDC of collateral tokens. This 1000USDC contributes to the value of kerosene tokens since it is counted in the TVL.
2. Alice tries to remove the 1 million USDC tokens. This would drop the value of kerosene tokens, and risk liquidating BOB.
3. BOB frontruns the withdrawal by depositing 1 wei of USDC in Alice's account. Alice cannot withdraw her funds, and the value of kerosene tokens remains high.
4. BOB effectively prevents himself from being liquidated by manipulating ALICE's account.

Since this is not just a griefing attack, but also profits the attacker by delaying/preventing liquidation, this is a high severity issue.

Tools Used

Manual Review

Recommended Mitigation Steps

Use the isDNftOwner modifier in deposit function to prevent users from carrying out this attack

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/44becc2f09c3a75bd548d5ec756a8e88a345e826/src/core/VaultManagerV2.sol#L134-L153

Vulnerability details

Impact

Users can put in kerosene tokens in vaults, and that kerosene is used to overcollateralize vault debts. There are two types of kerosene vaults: bounded and unbounded. Bounded vaults cannot be withdrawn out of, and has 2* the weight of unbounded vault deposits.

The issue is that the protocol doesnt properly handle withdrawals from unbounded kerosene vaults either. Unbounded kerosene vaults, by design are supposed to be able to be withdrawn from. To highlight the problem, lets look at the withdraw function in VaultManagerV2.sol:

    if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
    uint dyadMinted = dyad.mintedDyad(address(this), id);
    Vault _vault = Vault(vault);
    uint value = //...
                  10**_vault.oracle().decimals()
                  //...
    if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
    _vault.withdraw(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow();

This is the only function which does withdrawals from vaults. There is also the redeemDyad function, but that also runs into the same issues. As seen in the snippet, the _vault.oracle().decimals() is needed for the calculations.

However, the kerosene vaults dont have an oracle storage variable. This means users cannot pass in the address of kerosene vaults in the withdraw function to withdraw from, since it will revert when calling _vault.oracle().decimals(). This means the VaultManager provides no way to take out kerosene from unbounded kerosene vaults.

This basically makes unbounded kerosene vaults identical to kerosene vaults, with lower weight. Since this constitutes direct loss of funds, and is meant to operate differently, this is a medium severity issue.

The redeemDyad function also runs into the same issue, trying to call the oracle, which wont exist in kerosene vaults.

Proof of Concept

The contract can withdraw from vaults via the withdraw and redeemDyad functions. Both functions call the vault's oracle contract, which is not present in kerosene vaults.

Thus kerosene vaults cannot be withdrawn from.

Tools Used

Manual review

Recommended Mitigation Steps

Add a new withdrawKerosene function which allows withdrawals from kerosene vaults. This function should not call the oracle contract, and should be able to withdraw from only the unbounded vaults.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/44becc2f09c3a75bd548d5ec756a8e88a345e826/src/core/Vault.kerosine.unbounded.sol#L50-L69

Vulnerability details

Impact

The price of kerosene is calculated in the assetPrice function as shown below.

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());
    }
    uint numerator   = tvl - dyad.totalSupply();
    uint denominator = kerosineDenominator.denominator();
    return numerator * 1e8 / denominator;

Simplified, the price of kerosene is calculated as:

$kerosinePrice = \frac{TVL - dyadSupply}{kerosineSupply}$

Thus if the TVL ever falls below the dyad supply, it will lead to a revert.

The TVL is calculated with only the exo collateral, i.e. kerosine backing is not used to calculate the TVL. Also, the dyad amount that can be minted is capped by the non-kerosine collateral as shown in the VaultManagerV2.sol.

if (getNonKeroseneValue(id) < newDyadMinted)     revert NotEnoughExoCollat();

So, users can mint upto TVL of dyad. But if TVL ever drops below the dyad supply, the pricing mechanism breaks.

This is a very bad design, since users can toe the very extreme scenario even under normal conditions. If any volatility hits the markets, the TVL will drop, and assetPrice function will start reverting.

This will prevent liquidations from going through, since the collateralizaton ratio calculation calls assetPrice on the kerosene vault. So in case of volatility, mints, withdrawals and liquidations will all fail due to the pricing mechanism breaking.

Proof of Concept

Lets assume a scenario where the system has 2 users, Alice and Bob. Alice has 1 million USDC in their vault. Bob has 9 million USDC in their vault. Alice also has 600k kerosene tokens, and kerosene has a total supply of 10e6 (10 mill).

Thus TVL = 10 million, dyad_supply = 0. kerosene price = 10e6/10e6 = 1.

Now, Alice decides to mint 1 million dyad tokens (minus 1 wei, to pass checks).

Alice_dyad_mint = 1e6 -1 Alice_exo_collateral = 1e6

So the if (getNonKeroseneValue(id) < newDyadMinted) revert NotEnoughExoCollat(); check passes.

kerosene price = (10e6 - 1e6)/10e6 = 0.9 Alice_kerosene_collateral = 600k * 0.9 = 540k USD worth Alice_collateral_ratio = (1e6 + 540k)/(1e6) = 1.54

So the if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow(); check also passes.

Now, Bob removes his 9 million USDC of collateral.

Now, TVL = 1 million, dyad_supply = 1 million - 1 wei.

If the TVL reduces by even 1e-18, the assetPrice function will start reverting.

The main issue is that the system allowed ALICE to create a position where if all other players leave, Alice will be at the very extreme edge of the system. This design can be abused by users very easily.

Once assetPrice starts reverting, Alice cannot be liquidated anymore.

Tools Used

Manual Review

Recommended Mitigation Steps

Limit dyad to be minted upto 0.8* exo collateral. This will make it less capital efficient, but safer.

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/44becc2f09c3a75bd548d5ec756a8e88a345e826/src/core/Vault.kerosine.sol#L47-L59 https://github.com/code-423n4/2024-04-dyad/blob/44becc2f09c3a75bd548d5ec756a8e88a345e826/src/core/VaultManagerV2.sol#L225-L226

Vulnerability details

Impact

The protocol implements a flash-loan manipulation protection mechanism with the idToBlockOfLastDeposit variable. This values is set to the current block number during a deposit, and is checked during a withdrawal. If the system detects a deposit and withdrawal in the same block, the system reverts the transaction.

//function deposit
idToBlockOfLastDeposit[id] = block.number;

//function withdraw
if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();

The issue is that there is another way to move funds around: liquidations. This calls the move function to transfer around the balances, and does not update the idToBlockOfLastDeposit of the receiving account.

 function liquidate(
    uint id,
    uint to
  )
  {
    //...
    vault.move(id, to, collateral);
    //...
  }

So, a user can

1. Take out a flashloan. Deposit funds into a vault A. Mint dyad.
2. Manipulate the price of kerosene to trigger a liquidation.
3. Liquidate themselves and send their collateral to vault B.
4. Withdraw from vault B in the same block.
5. Pay off their flashloans.

The step 2 involves manipulating the price of kerosene, which affects their collateralization ratio. This has been discussed in a separate issue, and mainly states that if the user mints more dyad against free collateral in the system, or if any user takes out free collateral in the system, the price of kerosene will fall.

The flaw being discussed in this report is that the flash loan protection mechanism can be bypassed. This is different from the price manipulation issue and is thus a separate issue. Since this bypasses one of the primary safeguards in the system, this is a high severity issue.

Proof of Concept

The POC exploit setup requires 4 accounts: A, B, C and D. A is where the flashed funds will be deposited to. B is where the liquidated funds will be deposited to. C is for manipulating the kerosene price. D is for minting dyad at manipulated price to accrue bad debt in the system.

A is used to inflate the price of kerosene. B is used to bypass the flash loan protection mechanism. C is used to deflate the price of kerosene. D is used to mint dyad at the inflated price, accruing bad debt in the system and damaging the protocol.

1. Assume C has 1 million usdc tokens with 0 debt. C inflates the price of kerosene up by contributing to TVL, and will be used later to push the price down.
2. Alice takes out a flashloan of 10 million usdc tokens. She deposits them in account A.
3. Due to the sudden added massive flashloaned TVL, the internal price of kerosene shoots up.
4. Alice uses account D to mint out dyad tokens at this condition. Alice can now mint out exactly as many dyad tokens as her exo collateral. This is allowed since the price of kerosene is inflated, which covers the collateralization ratio. Alice effectively has a CR of cllose to 1.0 but the system thinks its 1.5 due to kerosene being overvalued. The system is still solvent at this point.
5. Alice buys kerosene from the market and adds it in account A and then mints dyad until account A has a CR of 1.5. The actual CR of A ignoring kerosene is close to 1.0.
6. Alice now removes collateral from account C. This reduces the price of C, making Account A liquidatable.
7. Alice now liquidates account A and sends the collateral to account B. This inflates the price of kerosene again since dyad supply has gone down, and she can repeat steps 5-6 multiple times since she can now mint more dyad tokens again.
8. Alice gets a large portion of her flashed funds into account B She withdraws them back out. This again drops the price of kerosene, allowing her to liquidate A more and and recover more of her funds into account B.
9. Alice pays back her flashloan.
10. Account D is now left with a CR close to 1.0, since the price of kerosene has now gone back to normal. Any price fluctuations in the exo collateral will now lead to bad debt.

This lets Alice open positions at a CR of close to 1.0. This is very dangerous in a CDP protocol, since Alice's risk is very low as she can sell off the minted dyad to recover her investment, but the protocol is now at a very risky position, close to accruing bad debt. Thus this is a high severity issue.

Tools Used

Manual Review

Recommended Mitigation Steps

The flashloan protection can be bypassed. MEV liquidation bots rely on flashloans to carryout liquidations, so there isnt a very good way to prevent this attack vector. Making the price of kerosene less manipulatable is a good way to lower this attack chance. However the system will still be open to flashloan deposits via liquidations.

Incorporating a mint fee will also help mitigate this vector, since the attacker will have a higher cost to manipulate the system.

Assessed type

Other

// SPDX-License-Identifier: MIT
pragma solidity =0.8.17;

import "forge-std/Test.sol";
import "forge-std/console.sol";
import {DeployBase, Contracts} from "../script/deploy/DeployBase.s.sol";
import {Parameters} from "../src/params/Parameters.sol";
import {DNft} from "../src/core/DNft.sol";
import {Dyad} from "../src/core/Dyad.sol";
import {Licenser} from "../src/core/Licenser.sol";
import {VaultManagerV2} from "../src/core/VaultManagerV2.sol";
import {Vault} from "../src/core/Vault.sol";
import {OracleMock} from "./OracleMock.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
import {IAggregatorV3} from "../src/interfaces/IAggregatorV3.sol";
import {ERC20} from "@solmate/src/tokens/ERC20.sol";

import {KerosineManager}        from "../src/core/KerosineManager.sol";
import {UnboundedKerosineVault} from "../src/core/Vault.kerosine.unbounded.sol";
import {BoundedKerosineVault}   from "../src/core/Vault.kerosine.bounded.sol";
import {Kerosine}               from "../src/staking/Kerosine.sol";
import {KerosineDenominator}    from "../src/staking/KerosineDenominator.sol";

contract VaultManagerV2Test is Test, Parameters {
  DNft         dNft;
  Licenser     vaultManagerLicenser;
  Licenser     vaultLicenser;
  Dyad         dyad;
  VaultManagerV2 vaultManagerV2;
 

  // weth
  Vault        wethVault;
  ERC20Mock    weth;
  OracleMock   wethOracle;

  //Kerosine
  Kerosine kerosine;
  UnboundedKerosineVault unboundedKerosineVault;

  KerosineManager        kerosineManager;
  KerosineDenominator    kerosineDenominator;
  
  //users
  address user1;
  address user2;

  function setUp() public {
    dNft       = new DNft();
    weth       = new ERC20Mock("WETH-TEST", "WETHT");
    wethOracle = new OracleMock(3000e8);

    vaultManagerLicenser = new Licenser();
    vaultLicenser        = new Licenser();

    dyad   = new Dyad(vaultManagerLicenser);

    //vault Manager V2
    vaultManagerV2    = new VaultManagerV2(
        dNft,
        dyad,
        vaultLicenser
      );

    //vault
       wethVault                   = new Vault(
        vaultManagerV2,
        ERC20(address(weth)),
        IAggregatorV3(address(wethOracle))
      );

    //kerosineManager
    kerosineManager = new KerosineManager();
    kerosineManager.add(address(wethVault));

    vaultManagerV2.setKeroseneManager(kerosineManager);

    //kerosine token
    kerosine = new Kerosine();
    //Unbounded KerosineVault
    unboundedKerosineVault = new UnboundedKerosineVault(
      vaultManagerV2,
      kerosine, 
      dyad,
      kerosineManager
    );
    

    //kerosineDenominator
    kerosineDenominator       = new KerosineDenominator(
      kerosine
    );
    unboundedKerosineVault.setDenominator(kerosineDenominator);

    //Licenser add vault
    vaultLicenser.add(address(wethVault));
    vaultLicenser.add(address(unboundedKerosineVault));

    //vaultManagerLicenser add manager
    vaultManagerLicenser.add(address(vaultManagerV2));

  }

function testFlashLoanAttackUsingLiquidateSimulation() public{
    wethOracle.setPrice(1000e8);
    //1 The attacker prepares two NFTs ,some collateral and some Kerosene Token.
    uint id = mintDNft();
    uint id_for_liquidator = mintDNft();
    weth.mint(address(this), 1e18);

    //2 deposit all the non-Kerosene collateral in the vault with One NFT like id=1 
    vaultManagerV2.add(id_for_liquidator, address(wethVault));
    weth.approve(address(vaultManagerV2), 1e18);
    vaultManagerV2.deposit(id_for_liquidator, address(wethVault), 1e18);

    //3 In the next blocknumber, flashloan non-Kerosene collateral from Lending like Aave,
    vm.roll(block.number + 1);
    weth.mint(address(this), 1e18);//Simulation borrow 1 weth

    //deposit all the borrowed flashloan non-Kerosene collateral and Kerosene Token in the vault with One NFT like id=0
    vaultManagerV2.add(id, address(wethVault));
    weth.approve(address(vaultManagerV2), 1e18);
    vaultManagerV2.deposit(id, address(wethVault), 1e18);

    vaultManagerV2.addKerosene(id, address(unboundedKerosineVault));
    kerosine.approve(address(vaultManagerV2), 1000_000_000e18);
    vaultManagerV2.deposit(id, address(unboundedKerosineVault), 1000_000_000e18);

    //Mint the max number Dyad you can
    vaultManagerV2.mintDyad(id, 1000e18, address(this));
    uint256 cr = vaultManagerV2.collatRatio(id); //2e18
    assertEq(cr, 2e18);

    //withdraw using id_for_liquidator,  manipulate the  Kerosene price
    vaultManagerV2.withdraw(id_for_liquidator, address(wethVault), 1e18, address(this));
    cr = vaultManagerV2.collatRatio(id); //1e18
    assertEq(cr, 1e18);
    //liquidate
    vaultManagerV2.liquidate(id, id_for_liquidator);
    //withdraw the vault which is move from id  using id_for_liquidator
    vaultManagerV2.withdraw(id_for_liquidator, address(wethVault), 1e18, address(this));
    console.log("weth balance is ", weth.balanceOf(address(this))/1e18);
}

  function mintDNft() public returns (uint) {
    return dNft.mintNft{value: 1 ether}(address(this));
  }

 function deposit(
    ERC20Mock token,
    uint      id,
    address   vault,
    uint      amount
  ) public {
    vaultManagerV2.add(id, vault);
    token.mint(address(this), amount);
    token.approve(address(vaultManagerV2), amount);
    vaultManagerV2.deposit(id, address(vault), amount);
  }

  receive() external payable {}

  function onERC721Received(
    address,
    address,
    uint256,
    bytes calldata
  ) external pure returns (bytes4) {
    return 0x150b7a02;
  }
}

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/44becc2f09c3a75bd548d5ec756a8e88a345e826/src/core/VaultManagerV2.sol#L205-L229

Vulnerability details

Impact

The docs state that the liquidation bonus should be 20%.

and in return receives an equivalent value plus a 20% bonus of the target Note’s backing colateral

This intent is also reflected in the constant values set in the system.

uint public constant LIQUIDATION_REWARD        = 0.2e18; //  20%

However, the way the math works out, the liquidation bonus is actually capped to 10%.

The liquidation bonus is calculated in two steps.

uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD);
uint liquidationAssetShare  = (liquidationEquityShare + 1e18).divWadDown(cappedCr);


// When liquidating
uint  collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare);

The first two lines calculate liquidationAssetShare, and the last line calculates the collateral amount to be transferred to the liquidator given this value. The issue is that this value is capped to 110%, which gives a 10% bonus only.

cappedCR can have a maximum value of 1.5. We are ignoring WAD mathematics here, since it can be shown in simple mathematics as well. Any higher value of cappedCR will not result in a liquidation. So cappedCR has to be less than 1.5, but her for the sake of simplicity lets assume it is exactly 1.5.

Then liquidationEquityShare is calculated as = (1.5 - 1)*0.2 = 0.1, Since LIQUIDATION_REWARD is 0.2.

Then liquidationAssetShare is calculated as = (0.1 + 1)/1.5 = 0.733

So if 100 dyad is being liquidated, that means the collateral backing it must be 150, since the CR is 1.5. In this case, the collateral paid out = 150*0.733 = 110.

So even in the case with the highest amount of liquidation reward, the liquidation bonus is capped to 10%.

This is assumed to be a case of a mathematical mistake, and is thus a medium severity issue.

Proof of Concept

In simple maths,

LIQUIDATION_REWARD=0.2, and cappedCR can have a max value of 1.5, which nets the max liquidation bonus.

liquidationEquityShare = (1.5-1)*0.2 = 0.1

liquidationAssetShare = (0.1+1)/1.5 = 0.733

For 1.5 units of collateral present, the liquidator is paid 0.733 * 1.5 = 1.1 units of collateral, so a bonus of 10% instead of 20%.

Tools Used

Manual review

Recommended Mitigation Steps

The liquidationEquityShare needs to be doubled.

uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD) * 2

Assessed type

Math

uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD);
uint liquidationAssetShare  = (liquidationEquityShare + 1e18).divWadDown(cappedCr);

uint  collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare);

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/44becc2f09c3a75bd548d5ec756a8e88a345e826/script/deploy/Deploy.V2.s.sol#L78-L92

Vulnerability details

Impact

The bounded kerosene vault actually calls the unbounded kerosene vault for its price, and just returns the double of that value.

function assetPrice()
    public
    view
    override
    returns (uint) {
      return unboundedKerosineVault.assetPrice() * 2;
  }

For this to work, the setUnboundedKerosineVault function needs to be called on the bounded kerosene vault. This is missing from the deployment script.

Since the deployment script is in scope, the deployed state should also be checked for configuration errors. This is a configuration error since the vault is deployed, but then not initialized enough to use its functionality properly. Thus this a medium severity issue.

Proof of Concept

The assetPrice function in bounded vaults will not function. It will revert on a call.

Tools Used

Manual review

Recommended Mitigation Steps

Call the setUnboundedKerosineVault function on the bounded vault with the unbounded vault's address.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/44becc2f09c3a75bd548d5ec756a8e88a345e826/src/core/VaultManagerV2.sol#L94-L104

Vulnerability details

Impact

The VaultManager.sol contract's remove function allows users to remove vaults from their own list of vaults. It first does a check to ensure there is no deposits in the vault still by that user.

if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();

The issue is that anyone can make deposits to any other user's vault. This is because the deposit function only does a isValidDNft check which only checks if the target vault exists, not if it is owned by the same person.

modifier isValidDNft(uint id) {
    if (dNft.ownerOf(id) == address(0))   revert InvalidDNft(); _;
  }

So any malicious user can deposit a single wei of tokens to a vault and prevent the vault owners from removing that vault. This is a griefing vector.

Proof of Concept

Alice wants to remove vault A from her list. She takes out all her collateral and calls remove(ALice_nft_id, vault_A_Address).

Bob frontruns Alice's remove calls and deposits 1 wei of tokens to vault A by calling deposit(Alice_nft_id,vault_A_Address,1).

Alice's transaction reverts.

Tools Used

Manual Review

Recommended Mitigation Steps

Change the isValidDNft modifier to a isDNftOwner check.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/44becc2f09c3a75bd548d5ec756a8e88a345e826/src/core/VaultManagerV2.sol#L172-L203

Vulnerability details

Impact

The function burnDyad is open for all, meaning that any user can remove dyad debt from any other user's account. The function only has the isValidDNft modifier, which checks if the nft exists, not the ownership.

The issue is that if a user wants to repay the complete debt of their own account, by either calling burnDyad or redeemDyad, another user can call burnDyad on their account and burn 1 wei frontrunning the transaction. This will cause the owner's transaction to revert, since the system will be trying to burn more debt than the user accrued.

function burnDyad(
    uint id,
    uint amount
  )
    external
      isValidDNft(id)
  {
    dyad.burn(id, msg.sender, amount);
    emit BurnDyad(id, amount, msg.sender);
  }

The dyad contract ensures that users cannot burn more tokens than they minted via an underflow protection.

_burn(from, amount);
mintedDyad[msg.sender][id] -= amount;

So users will be griefed and will be unable to pay off their full debt. This is a griefing attack and is thus a medium severity issue.

Proof of Concept

Assume Alice has a debt of 100 dyad. She calls burnDyad with 100. Bob frontruns this transaction and pays off 1 wei of Alice's debt. Alice's transaction now fails, due to her trying to repay more debt than she has.

Tools Used

Manual review

Recommended Mitigation Steps

Add a code section that truncates the amount to pay in case a larger amount is passed in for both burnDyad and redeemDyad.

if(amount > dyad.mintedDyad(address(this), id)) amount = dyad.mintedDyad(address(this), id);

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/44becc2f09c3a75bd548d5ec756a8e88a345e826/src/core/KerosineManager.sol#L4-L52

Vulnerability details

Impact

The kerosene manager is the contract responsible for managing kerosene prices. In the current state it has broken functionality due to the design.

The KeroseneManager contract contains a list of vaults. When we look at UnboundedKerosineVault contract, we see what those vaults are used for.

function assetPrice()
    public
    view
    override
    returns (uint) {
      // ...
      address[] memory vaults = kerosineManager.getVaults();
      uint numberOfVaults = vaults.length;
      for (uint i = 0; i < numberOfVaults; i++) {
        Vault vault = Vault(vaults[i]);
        // ...
                * vault.assetPrice() * 1e18
        // ...
      }

The KeroseneManager contract is expected to have a list of the backing vaults. This list is then queried for the individual assetPrice(). Crucially, the KeroseneManager contract will not have the vault which takes kerosene as the asset. This is because calling assetPrice on a vault handling kerosene will make it go into an infinite loop.

So this section of the code expects the KeroseneManager contract to only contain a list of the vaults which has exo collateral.

Now let's look at VaultManagerV2.sol contract's getKeroseneValue function. This function is supposed to return the value of kerosene tokens a user has deposited. It should do this by querying all the vaults which takes keroesne as the deposit.

function getKeroseneValue(
    uint id
  )
    public
    view
    returns (uint) {
      uint numberOfVaults = vaultsKerosene[id].length();
      for (uint i = 0; i < numberOfVaults; i++) {
        Vault vault = Vault(vaultsKerosene[id].at(i));
        if (keroseneManager.isLicensed(address(vault))) {
          usdValue = vault.getUsdValue(id);
        }
        //...
  }

So the vaultsKerosene should hold the vaults which take keorsene as its deposit. But then the code calls keroseneManager.isLicensed(address(vault). The isLicensed function only checks in the same vaults array in the kerosene manager.

function isLicensed(
    address vault
  )
    return vaults.contains(vault);

So, this expects the KeroseneManager to also contain the kerosene accepting vaults as well!

So we have shown that in UnboundedKerosineVault, the KeroseneManager contract is expected to have only the backing vaults, not the keresone deposit vaults itself, or it will enter an infinite loop.

We have also shown that in VaultManagerV2, the KeroseneManager contract is expected to have the kerosene deposit vaults as well, or it will not be able to pass the isLicensed check.

Both the above statements cannot be true at the same time. This is a design flaw. The KeroseneManager contract, if it contains the kerosene deposit vaults will break the kerosene pricing mechanism, and if it does not, it will break the manager contract.

Thus, the current design is flawed and will break the functionality.

If we look at the deployment script, we see that the kerosene manager only has the backing vaults.

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

This means the manager's isLicensed call will fail for vaults which accept kerosene.

Proof of Concept

This is a design flaw as described above. A proof of concept cannot be shown since its broken functionality.

Tools Used

Manual Review

Recommended Mitigation Steps

Store the kerosene vaults info in the Licenser contract. Then change the keroseneManager.isLicensed call to vaultLicenser.isLicensed in the manager contract.

Assessed type

Error

    KerosineManager kerosineManager = new KerosineManager();

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

Lines of code

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

Vulnerability details

Impact

The value of kerosene tokens is calculated according to the following formula:

$kerosenePrice = \frac{TVL - dyadSupply}{keroseneSupply}$

So the price of kerosene changes based on user actions. If a user has a large amount of tokens in their vault, that adds to the TVL. Now if a user mints a bunch of dyad tokens with their deposit as collateral, or if a removes a bunch of their unused collateral, the price of kerosene tokens will drop instantaneously.

When calculating the collateralization ratio of a user's position, the price of kerosene is essential. This is used in the getKeroseneValue function.

function getKeroseneValue(
    uint id
  )
    public
    view
    returns (uint) {
      uint totalUsdValue;
      uint numberOfVaults = vaultsKerosene[id].length();
      for (uint i = 0; i < numberOfVaults; i++) {
        Vault vault = Vault(vaultsKerosene[id].at(i));
        uint usdValue;
        if (keroseneManager.isLicensed(address(vault))) {
          usdValue = vault.getUsdValue(id);
        }
        totalUsdValue += usdValue;
      }
      return totalUsdValue;
  }

function getTotalUsdValue(
    uint id
  )
    public
    view
    returns (uint) {
      return getNonKeroseneValue(id) + getKeroseneValue(id);
  }

As seen above, thetotalUsdValue depends on the getKeroseneValue function, which calls getUsdValue on the kerosene vaults. So instantaneously dropping the price of kerosene tokens will also instantaneously drop the totalUSDValue of the user's position, decreasing their collateralization ratio. This can be used to force users into liquidation, if their final ratio drops below the liquidation threshold.

Since this allows any user to affect the collateralization ratio of other user's positions, this is a high severity issue.

Proof of Concept

Assume Alice has 1 million USDC tokens in her vault, and 0 minted dyad tokens. TVL is 10 million USD of exo collateral, dyad supply is 5.5 million. Assume total supply of kerosene is 100,000.

So the price of kerosene is:

$kerosenePrice = \frac{10,000,000 - 5,500,000}{100,000} = 45 USD$

Now assume Bob has an account with 1000 USDC tokens, and 20 kerosene tokens. They minted 1150 dyad tokens. These values were already included in the calculation above, so the minted amounts wont change. Their current collateralization ratio is:

$CR = \frac{1000 + 45 \times 20}{1150} = 1.65$

Now, Alice decides to attack BOB's position. She withdraws her 1 million USDC tokens from her vault. This drops the TVL to 9 million USD. Now the price of kerosene is:

$kerosenePrice = \frac{9,000,000 - 5,500,000}{100,000} = 35 USD$

Now the collateralization ratio of Bob's position is:

$CR = \frac{1000 + 35\times 20}{1150} = 1.48$

Thus Bob's position is liquidatable.

Thus whales can set up traps by putting in large amounts of capital as TVL into the vault, propping up the price of kerosene. Then whenever a user opens a position which is moderately close to the liquidation threshold, the whale can immediately take out their liquidity, effectively doing a rug pull, and liquidate the users.

Since this gives high net worth users an easy way to manipulate the protocol at no loss, this is a high severity issue.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a TWAP mechanism to calculate the price of kerosene. The price of kerosene being manipulatable instantaneously is a huge risk. By using a TWAP mechanism, the price of kerosene will be more stable, and users will have more time to react to changes in the price of kerosene.

Assessed type

Oracle