DYAD - steadyman's results

Lines of code

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

Vulnerability details

POC And Impact

In VaultManagerV2, when the user calls the deposit function, the current block.number will be recorded. When the user calls the withdraw function, the current block.number will be checked to prevent reentrancy.

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

When the user calls the withdraw function, the attacker can call the deposit function in advance, with the parameter amount being 0, which will prevent the user from making withdrawals

Tools Used

foundry, Manual audit

Recommended Mitigation Steps

Define a variable Lock to prevent reentrancy.

+ uint private Lock;

  function deposit(
    uint    id,
    address vault,
    uint    amount
  ) 
    external 
      isValidDNft(id)
  {
-   idToBlockOfLastDeposit[id] = block.number;
+   Lock = 1;
    Vault _vault = Vault(vault);
    _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
    _vault.deposit(id, amount);
+   Lock = 0;
  }

  /// @inheritdoc IVaultManager
  function withdraw(
    uint    id,
    address vault,
    uint    amount,
    address to
  ) 
    public
      isDNftOwner(id)
  {
-   if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
+   if (Lock == 1) revert DepositedInSameBlock();
    uint dyadMinted = dyad.mintedDyad(address(this), id);
    Vault _vault = Vault(vault);
    uint value = amount * _vault.assetPrice() 
                  * 1e18 
                  / 10**_vault.oracle().decimals() 
                  / 10**_vault.asset().decimals();
    if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
    _vault.withdraw(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
  }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Users can deposit or withdraw Kerosine to UnboundedKerosineVault through the deposit() and withdraw() methods of VaultManagerV2. When the user calls the withdraw() method to take out Kerosine, the transaction will be reverted. The reason why the transaction will be reverted is that the pricing methods of Vault and UnboundedKerosineVault are different. In Vault, the price is obtained through the oracle instance, but in UnboundedKerosineVault, there is no oracle instance. In withdraw(), the oracle is called to obtain the decimal value of the price when calculating the price, but there is no oracle instance in UnboundedKerosineVault, which causes the transaction to be reverted.

Proof of Concept

function testVaultManagerWithdraw() public {
    testLicenseVaultManager();

    address addr1 = makeAddr("addr1");
    deal(MAINNET_KEROSENE, addr1, 100e18);

    vm.startPrank(MAINNET_OWNER);
    uint NFTid = DNft(MAINNET_DNFT).mintInsiderNft(addr1); 
    vm.stopPrank();

    vm.startPrank(addr1);
    IERC20(MAINNET_KEROSENE).approve(address(contracts.vaultManager), 100e18);

    vm.ro11(1);

    contracts.vaultManager.deposit(
        NFTid,
        address(contracts.unboundedKerosineVault), 
        100e18
    );

    vm.ro11(2);

    vm.expectRevert();
    contracts.vaultManager.withdraw(
        NFTid,
        address(contracts.unboundedKerosineVault),
        100e18,
        addr1
    );
    vm.stopPrank();
}

Tools Used

foundry, Manual audit

Recommended Mitigation Steps

Add a bool variable to distinguish whether the user wants to retrieve Kerosine.

  function withdraw(
    uint    id,
    address vault,
    uint    amount,
    address to,
    bool isKerosine  
  ) 
    public
      isDNftOwner(id)
  {
    if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
    uint dyadMinted = dyad.mintedDyad(address(this), id);
    Vault _vault = Vault(vault);
    if(!isKerosine) {
    uint value = amount * _vault.assetPrice() 
                  * 1e18 
                  / 10**_vault.oracle().decimals() 
                  / 10**_vault.asset().decimals();
    if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
    }esle{ 
    if (getNonKeroseneValue(id) - amount< dyadMinted) revert NotEnoughExoCollat();
    }
    _vault.withdraw(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
  }

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

When using DeployV2, the deployed Dyad contract is used, which will cause TVL > DYAD totalsupply to not be established, resulting in system DoS. The reason why Dos occurs is because the total supply of Dyad on the Ethereum mainnet is very large, but all vaults are redeployed, resulting in TVL being 0. At this time, TVL < DYAD totalsupply, so when using the assetPrice function in the UnboundedKerosineVault contract to calculate the price, it will An underflow occurred.

uint numerator   = tvl - dyad.totalSupply();

This will result in users using UnboundedKerosineVault being unable to perform any operations. Any attempt to interact with it will fail.

Proof of Concept

//The reason why the getNonKeroseneValue function was chosen for testing is because there is a serious problem with the addKerosene of VaultManagerV2, which prevents the Kerosene vault from being added.
//However, add can add the Kerosene vault, so the getNonKeroseneValue function is //selected for testing.
function testgetNonKeroseneValueRevert () public {

    testLicenseVaultManager();

    address addr1 = makeAddr ("addr1");

    vm.startPrank(MAINNET_OWNER);
    uint NFTid = DNft(MAINNET_DNFT).mintInsiderNft(addr1);
    IERC20(MAINNET_KEROSENE).transfer(addr1, 100e18);
    vm.stopPrank();

    v.startPrank(addr1);
    contracts.vaultManager.add(NFTid, address(contracts.unboundedKerosineVault));
    IERC20(MAINNET_KEROSENE).approve(address(contracts.vaultManager), 10e18);
    contracts.vaultManager.deposit(
        NFTid,
        address(contracts.unboundedKerosineVault),
        10e18
    );
    vm.expectRevert();
    contracts.vaultManager.getNonKeroseneValue(NFTid)
    vm.stopPrank();
}

Tools Used

foundry, Manual audit

Recommended Mitigation Steps

Redeploy a Dyad contract and provide a method for users to transfer their old Dyad Token and assets in the old vault.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

In the addKerosene function of VaultManagerV2, it is checked whether the vault is added by keroseneManager.

However, the Vaults in keroseneManager are used to store ordinary vaults used to calculate LVT, not Kerosene vaults.

In Deploy.V2.sol, all vaults are added to the License, and ethVault and wstEth are added to the kerosineManager.

This will result in:

1. Users can add Kerosene vaults to Vaults, causing Kerosene to be calculated in getNonKeroseneValue function.
2. Users can repeatedly add ordinary vaults to vaultsKerosene, causing asset values to be double calculated.

Proof of Concept

function testAddError() public {
    testLicenseVaultManager();

    address addr1 = makeAddr("addr1");

    deal(MAINNET_WETH, addr1, 100e18);

    vm.startPrank(MAINNET_OWNER);
    uint256 NFTid = DNft(MAINNET_DNFT).mintInsiderNft(addr1);
    IERC20(MAINNET_KEROSENE).transfer(addr1, 100e18);
    vm.stopPrank();

    vm.startPrank(addr1);
    contracts.vaultManager.add(NFTid, address(contracts.ethVault));
    IERC20(MAINNET_WETH).approve(address(contracts.vaultManager), 10e18);
    contracts.vaultManager.deposit(NFTid, address(contracts.ethVault), 10e18);
    console.log("Before repeat add ethVault totalValue:");
    console.log(contracts.vaultManager.getTotalUsdValue(NFTid));

    contracts.vaultManager.addKerosene(NFTid, address(contracts.ethVault));
    console, log("After repeat add ethVault totalValue:");
    console. log(contracts.vaultManager.getTotalUsdValue(NFTid));
    vm.stopPrank();
}

Tools Used

foundry, Manual audit

Recommended Mitigation Steps

Add mapping for managing Kerosene treasury in Licenser contract

Licenser.sol

+  mapping (address => bool) public isKeroseneLicensed; 

+  function addKerosene   (address vault) external onlyOwner { isKeroseneLicensed[vault] = true; }
+  function removeKerosene (address vault) external onlyOwner { isKeroseneLicensed[vault] = false; }

VaultManagerV2.sol

  function addKerosene(
      uint    id,
      address vault
  ) 
    external
      isDNftOwner(id)
  {
    if (vaultsKerosene[id].length() >= MAX_VAULTS_KEROSENE) revert TooManyVaults();
-   if (!keroseneManager.isLicensed(vault))                 revert VaultNotLicensed();
+   if (!isKeroseneLicensed.isLicensed(vault))              revert VaultNotLicensed();
    if (!vaultsKerosene[id].add(vault))                     revert VaultAlreadyAdded();
    emit Added(id, vault);
  }

Assessed type

Other