DYAD - 0x486776's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L64-L65 https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L93-L96 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L67-L91 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L156-L169 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L230-L286

Vulnerability details

Impact

Users can add collateral vaults to both vaults[id] and vaultsKerosene[id]. As a result, their collateral is calculated twice, leading to incorrect actions within the system.

Proof of Concept

As observed in lines L64, L65, L93, and L94 of Deploy.V2.s.sol, collateral vaults are added to both kerosineManager and vaultLicenser.

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

        [...]

93      vaultLicenser.add(address(ethVault));
94      vaultLicenser.add(address(wstEth));
95      vaultLicenser.add(address(unboundedKerosineVault));
        // vaultLicenser.add(address(boundedKerosineVault));

Consequently, users can add collateral vaults to both vaults[id] and vaultsKerosene[id] , resulting in their collaterals being calculated twice as large, leading to incorrect system operations.

Let's walk through this scenario:

1. Alice adds ethVault, wstEth to both vaults[id] and vaultsKerosene[id]. The restrictions at L75 and L78 do not prevent Alice from doing this because ethVault and wstEth have already been added to keroseneManager and vaultLicenser.

    function add(
        uint    id,
        address vault
    ) 
      external
        isDNftOwner(id)
    {
      if (vaults[id].length() >= MAX_VAULTS) revert TooManyVaults();
75    if (!vaultLicenser.isLicensed(vault))  revert VaultNotLicensed();
      if (!vaults[id].add(vault))            revert VaultAlreadyAdded();
      emit Added(id, vault);
    }

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

3. Alice deposits $1000 worth of ETH into ethVault and $1000 worth of WSTETH into the vault wstEth.
4. Next, Alice calls VaultManagerV2::mintDyad with the second parameter amount set to $2000 and receives $2000 dyad.

    function mintDyad(
      uint    id,
      uint    amount,
      address to
    )
      external 
        isDNftOwner(id)
    {
      uint newDyadMinted = dyad.mintedDyad(address(this), id) + amount;
      if (getNonKeroseneValue(id) < newDyadMinted)     revert NotEnoughExoCollat();
      dyad.mint(id, to, amount);
167   if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow(); 
      emit MintDyad(id, amount, to);
    }

VaultManagerV2::mintDyad should revert the transaction because the collateral ratio is calculated as (1000 + 1000) / 2000 = 1, which is less than MIN_COLLATERIZATION_RATIO = 1.5. However, the value of collatRatio(id) at L167 in VaultManagerV2::mintDyad is calculated as 2, results in a success transaction.

This discrepancy occurs because:

• VaultManagerV2::collatRatio is calculated using getTotalUsdValue(id).

    function collatRatio(
      uint id
    )
      public 
      view
      returns (uint) {
        uint _dyad = dyad.mintedDyad(address(this), id);
        if (_dyad == 0) return type(uint).max;
238     return getTotalUsdValue(id).divWadDown(_dyad);
    }

• getTotalUsdValue(id) is determined by getNonKeroseneValue(id) + getKeroseneValue(id).

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

• While getNonKeroseneValue(id) should be $2000 and getKeroseneValue(id) should be $0, getKeroseneValue(id) is calculated as $2000 instead of $0 because ethVault and wstEth are already in vaultsKerosene[id].

  function getKeroseneValue(
    uint id
  ) 
    public 
    view
    returns (uint) {
      uint totalUsdValue;
276   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;
  }

As a result, Alice's collateral ratio is calculated as 2 instead of 1, and she will not be liquidated even if her actual collateral is insufficient for liquidation.

Tools Used

Manual review

Recommended Mitigation Steps

In Deploy.V2.s.sol, collateral vaults should be added only into vaultLicenser, not into kerosineManager. And unboundedKerosineVault should be added into kerosineManager, not into vaultLicenser.

- 64:     kerosineManager.add(address(ethVault));
- 65:     kerosineManager.add(address(wstEth));
+ 64:     kerosineManager.add(address(unboundedKerosineVault));


        [...]

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

Above recommendation is not perfect, because Vault.kerosine.unbounded::assetPrice is based on kerosineManager.getVaults(). So, Vault.kerosine.unbounded::assetPrice should be improved as well.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

An attacker can undo any legitimate withdrawals and redemption.

Proof of Concept

In VaultManagerV2::withdraw, there is a validation for idToBlockOfLastDeposit[id] at L143.

    function withdraw(
        uint    id,
        address vault,
        uint    amount,
        address to
    ) 
        public
        isDNftOwner(id)
    {
143     if (idToBlockOfLastDeposit[id] == block.number) 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(); 
    }

If an attacker sets idToBlockOfLastDeposit[id] to the current block number through front-running, then the owner of id will be unable to withdraw his assets.

Let's consider the following scenario:

1. Alice, the owner of id, initiates a transaction to call VaultManagerV2::withdraw.
2. Bob then quickly makes a transaction to call VaultManagerV2::deposit with the 3rd parameter amount set to 1 wei, using front-running.

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

As indicated at L127 of VaultManagerV2::deposit, Bob sets idToBlockOfLastDeposit[id] to the current block number. Consequently, Alice's transaction is reverted at L143 of VaultManagerV2::withdraw.

Redemption is also impossible since the function VaultManagerV2::redeemDyad calls the function VaultManagerV2::withdraw.

Tools Used

Manual review

Recommended Mitigation Steps

There are 2 ways to fix this problem.

1. Only the owner of id can deposit into id.

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

2. Remove the block number validation.

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

    function withdraw(
        uint    id,
        address vault,
        uint    amount,
        address to
    ) 
        public
        isDNftOwner(id)
    {
-       if (idToBlockOfLastDeposit[id] == block.number) 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/main/src/core/VaultManagerV2.sol#L134-L153

Vulnerability details

Impact

Users can't withdraw their kerosene.

Proof of Concept

In VaultManagerV2::withdraw, there is a calculation of value to be withdrawn at L146.

    function withdraw(
        uint    id,
        address vault,
        uint    amount,
        address to
    ) 
        public
        isDNftOwner(id)
    {
        if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
        uint dyadMinted = dyad.mintedDyad(address(this), id);
        Vault _vault = Vault(vault);
146     uint value = amount * _vault.assetPrice() 
                    * 1e18 
148                 / 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(); 
    }

During the calculation, it calls _vault.oracle().decimals() at L148. However, if vault is a kerosene vault, there is no oracle. Therefore, this calculation is reverted, resulting in the withdrawal of kerosene being reverted as well.

Tools Used

Manual review

Recommended Mitigation Steps

Withdrawing kerosene never affects the value of getNonKeroseneValue(id), so the calculation of the value of kerosene to be withdrawn is unnecessary.

    function withdraw(
        uint    id,
        address vault,
        uint    amount,
        address to
    ) 
        public
        isDNftOwner(id)
    {
        if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
        uint dyadMinted = dyad.mintedDyad(address(this), id);
        Vault _vault = Vault(vault);
-       uint value = amount * _vault.assetPrice() 
+       uint value;
+       if(vaults[id].contains(vault)){
+           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

Other

Lines of code

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

Vulnerability details

Impact

Anyone can manipulate the price of kerosene and keep their DNFT token healthy.

Proof of Concept

Anyone can mint his own DNFT token and deposit a large amount of collateral into it. Then, the total collateral value of the entire system becomes significantly larger, results in a large price of kerosene. Consequently, many DNFT tokens exit from liquidation. Even, for some DNFT tokens, although the collateral value falls below the dyad amount minted via those DNFT tokens, it is not liquidated since it has expensive kerosenes. Then, if the situation improves, he can safely withdraw all his collateral in his new DNFT token since there is no dyad minted via that id.

Tools Used

Manual review

Recommended Mitigation Steps

The liquidating condition should be improved.

    function liquidate(
      uint id,
      uint to
    ) 
      external 
        isValidDNft(id)
        isValidDNft(to)
      {
        uint cr = collatRatio(id);
-       if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();
+       if (cr >= MIN_COLLATERIZATION_RATIO && getNonKeroseneValue(id) >= dyad.mintedDyad(address(this), id)) revert CrTooHigh();
        dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id));

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

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

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The price of Kerosence is calculated significantly smaller than it should be. Consequently, this decreases the collateral ratios, leading to the unfair liquidation of many legitimate ids.

Proof of Concept

In Vault.kerosine.unbounded::assetPrice, the calculation of numerator is incorrect at L65.

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

In fact, in the calculation of numerator at L65, tvl should be subtracted by the total amount of dyad minted through VaultManagerV2. As a result, the return value of Vault.kerosine.unbounded::assetPrice is significantly smaller than it should be. This directly affects the value of VaultManagerV2::getKeroseneValue, causing the VaultManagerV2::collatRatio to return a much smaller value, which can unfairly lead to the liquidation of legitimate ids.

Tools Used

Manual review

Recommended Mitigation Steps

At L65 of Vault.kerosine.unbounded::assetPrice, dyad.totalSupply() should be replaced by the total amount of dyad minted through VaultManagerV2.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

If the total collateral value falls below the total dyad amount, liquidation is not possible.

Proof of Concept

If the total collateral value is smaller than total dyad amount, Vault.kerosine.unbounded::assetPrice will be reverted at L65.

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

This directly affects Vault.kerosine::getUsdValue, causing it to be reverted. Subsequently VaultManagerV2::collatRatio is also reverted, which leads to the impossibility of liquidation since VaultManagerV2::liquidate calls VaultManagerV2::collatRatio.

Tools Used

Manual review

Recommended Mitigation Steps

If the total collateral value is smaller than total dyad amount, Vault.kerosine.unbounded::assetPrice should return 0.

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

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The liquidator could incur loss during liquidation.

Proof of Concept

As you can see at L221 of VaultManagerV2::liquidate, only collateral in vaults is moved to to, not kerosenes in vaultsKerosene.

    function liquidate(
      uint id,
      uint to
    ) 
      external 
        isValidDNft(id)
        isValidDNft(to)
      {
        uint cr = collatRatio(id);
        if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();
215     dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id));

        uint cappedCr               = cr < 1e18 ? 1e18 : cr;
        uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD);
219     uint liquidationAssetShare  = (liquidationEquityShare + 1e18).divWadDown(cappedCr);

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

Let's follow this scenario:

1. Alice, the owner of id, has $2500 of collateral, $500 of kerosene, and $2400 of dyad. Her collateral ratio is (2500 + 500) / 2400 = 1.25, indicating she is facing liquidation.
2. Bob calls VaultManagerV2::liquidate.
3. Bob burns his $2400 dyad at L215.
4. At L219, liquidationAssetShare is calculated as ((1.25 - 1) * 0.2 + 1) / 1.25 = 0.84.
5. 84% of Alice's collateral is then moved by looping through Alice's vaults(L221), not vaultsKerosene.

Therefore, Bob incurs a loss of $2400 - $2500 * 0.84 = $300.

Tools Used

Manual review

Recommended Mitigation Steps

Liquidation should move Kerosenes as well.

    function liquidate(
      uint id,
      uint to
    ) 
      external 
        isValidDNft(id)
        isValidDNft(to)
      {
        uint cr = collatRatio(id);
        if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();
        dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id));

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

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

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

        emit Liquidate(id, msg.sender, to);
    }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

If the collateral ratio of id is smaller than 100%, nobody wants to liquidate it. As a result, the entire protocol doesn't work well.

Proof of Concept

If cr < 1e18 in VaultManagerV2::liquidate, liquidationEquityShare will be 0 at L218.

    function liquidate(
      uint id,
      uint to
    ) 
      external 
        isValidDNft(id)
        isValidDNft(to)
      {
        uint cr = collatRatio(id);
        if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();
        dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id));

217     uint cappedCr               = cr < 1e18 ? 1e18 : cr;
218     uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD);
        uint liquidationAssetShare  = (liquidationEquityShare + 1e18).divWadDown(cappedCr);

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

So, the liquidator will incur loss. As a result, nobody wants to liquidate this id. Then, from that id, redeeming or withdrawing can't be done.

Tools Used

Manual review

Recommended Mitigation Steps

There should be a mechanism to process ids whose collateral ratio is smaller than 100%.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L94-L104 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L119-L131 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.sol#L42-L51

Vulnerability details

Impact

An attacker can reverse any vault removal, causing the owner of id to be unable to remove unnecessary vaults.

Proof of Concept

In VaultManagerV2::remove, there is a validation for Vault(vault).id2asset(id) at L101.

    function remove(
        uint    id,
        address vault
    ) 
        external
        isDNftOwner(id)
    {
101     if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();
        if (!vaults[id].remove(vault))     revert VaultNotAdded();
        emit Removed(id, vault);
    }

If an attacker sets Vault(vault).id2asset(id) to a nonzero value through front-running, then the owner of id will be unable to remove the vault.

Let's consider the following scenario:

1. Alice, the owner of id, initiates a transaction to call VaultManagerV2::remove.
2. Bob then quickly makes a transaction to call VaultManagerV2::deposit with the 3rd parameter amount set to 1 wei, using front-running.

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

As indicated at L130 of VaultManagerV2::deposit, it triggers Vault::deposit(id, 1).

    function deposit(
        uint id,
        uint amount
    )
        external 
        onlyVaultManager
    {
49      id2asset[id] += amount;
        emit Deposit(id, amount);
    }

Then, at L49 of Vault::deposit, id2asset[id] is set to 1. This results in Alices's transaction being reverted at L101 of VaultManagerV2::remove.

Tools Used

Manual review

Recommended Mitigation Steps

VaultManagerV2::deposit should only be callable by the owner of id.

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

Assessed type

Other