DYAD - bhilare_'s results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L66-L78 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Licenser.sol#L12 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L93-L96

Vulnerability details

Impact

Users can mint without providing any exogenous collateral, and can mint more than intended by the protocol, since kerosene vault can be added in NON kerosene/exogenous vault. Which in turn can also cause Kerosene's value to go down, possibly 0 too.

Proof of Concept

While adding vault (non-kerosene/exogenous) using VaultManager::add, it is being checked whether the vault is licensed or not, if it is licensed, then that vault is being added.

if (!vaultLicenser.isLicensed(vault))  revert VaultNotLicensed();

In order a vault to be licensed, the vault should be added in the License contract. Now if we see the deploy script :

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

You can see unboundedKerosineVault is also being added.

Which means that any user can also add kerosene vault in non-kerosene vaults.

Consider a scenario.

Alice adds unboundedKerosine vault in Kerosene vault using addKerosene, and deposits 1000 $ worth kerosene in it. So till now , the total collateral is 1000$

Now afterwards as I said above, a kerosene vault can also be added in non-kerosene vaults i.e VaultManagerV2::vaults mapping, she added this unbounded kerosene vault here in exogenous vault also, using VaultManagerV2::add function

Now here, she didn't deposited any exogenous collateral, still getNonKeroseneValue(id), would return 1000 value (1000e8). And getKeroseneValue(id) would also return 1000 value (1000e8).

Which means here total collateral would be showed equal to 2000 , where she only deposited 1000 $ worth collateral.

Now Alice can mint at max 1000 DYAD , because of :

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

without providing any exogenous collateral.

And also , considering the CR value threshold of 1.5, if she would have deposited 1000 worth total collateral she should have only able to mint 1000/1.5 = 666.66 DYAD tokens (considering she has exo collateral > 666.66)

But she was able to mint 1000 tokens which is more than intended by protocol.

Also since the value of Kerosene is being calculated ,where numerator is TVL - dyadMinted, and in TVL only exogenous vaults are included.

It would cause kerosene value to go down, and can also be a case where dyadMinted > TVL , and kerosene price would go down to 0, if more people just deposit kerosene in exogenous vaults just like Alice did.

Which would be even more serious issue.

Tools Used

Manual Review

Recommended Mitigation Steps

For checking whether while adding exogenous vaults, the vault is licensed or not, a different implementation should be added, where unboundedKerosineVault isn't licensed just for adding into non exo vaults. Which would mean that unboundedKerosineVault is licensed, but not allowed to be added in the exo vaults i.e. VaultManagerV2::vaults mapping.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Everytime while requesting withdrawal of kerosene, the users won't be able to withdraw any amount of kerosene because it will always give runtime error.

Proof of Concept

[ I DID SUBMIT SIMILAR ISSUE THAT USERS WON'T BE TO WITHDRAW THEIR KEROSENE, BUT THERE THE ROOT CAUSE WAS DIFFERENT. THERE THE ISSUE IS BECAUSE OF EXO COLLATERAL CHECK. HERE THE ISSUE REASON IS DIFFERENT.]

In withdraw function :

/// @inheritdoc IVaultManager
  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(); 
  }

The provided vault's contract instance is being created to interact with the deployed contract

And then while calculating value, _vault.oracle().decimals() is being called.

The issue is, Kerosine vault and exogenous vaults do have same interface, but in their implementation , Kerosine vault don't have any oracle storage variable implemented. https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.kerosine.sol https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/Vault.kerosine.unbounded.sol

Hence , since in the storage lookout no oracle named storage variable will be found, and hence will get a runtime error.

Hence user won't be able to withdraw their kerosene.

Tools Used

Manual review

Recommended Mitigation Steps

There should be a different implementation of withdrawing kerosene, where the value is not being calculated using this current implementation.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Users won't be able to withdraw their unbounded kerosene, even if they have enough exogenous collateral. Hence causing loss of funds , since the kerosene will be stucked in contract/unbounded kerosene vault, until unless the user burns DYAD, which he/she shouldn't have to, considering that he/she did had enough collateral and had no reason to burn his/her DYAD tokens.

Proof of Concept

If any user deposits kerosene in unbounded kerosene vault, then they can withdraw it afterwards if they want to, but they can't withdraw if they deposit in bounded kerosene vault

But the issue is while withdrawing from VaultManagerV2::withdraw:

/// @inheritdoc IVaultManager
  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(); 
  }

Because of the check if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();

Even if the user would be having enough exo collateral , they still won't be able to withdraw kerosene.

FOR EXAMPLE :

Alice has Exo collateral : 1000 $ worth

Kerosene deposited : 1000 $ worth DYAD Minted : 200.

Now here CR is 2000 / 200 = 10 . Which is safe.

Now if alice wants to withdraw her all kerosine. Since after that the CR would still be safe since then CR = 1000/200 = 5. She should be able to withdraw all kerosene.

Therefore here : getNonKeroseneValue(id) = 1000 value = 1000 dyadMinted = 200

In the check of exo collateral, here the inequality check would be 0 < 200 , and hence it would always revert with reason Not enough exo , even if she had enough exo collateral.

Hence then she won't be able to withdraw more than 800 $ worth kerosene, in this case.

And would have to burn DYAD , just to withdraw the remaining kerosene, which shouldn't have happened because she did have enough exogenous collateral.

Tools Used

Manual Review

Recommended Mitigation Steps

If kerosene is being requested to withdraw then this check of whether there's enough exogenous collateral or not after withdrawing should NOT be added, because user is not withdrawing any exogenous collateral.

That just don't make sense to check exogenous collateral if endogenous/kerosene collateral is being withdrawed.

Hence the vault being passed as parameter should be checked whether its unbounded kerosine vault or not. And then if-else statement should be implied, where if its not a kerosine vault then the ENOUGH EXO COLLATERAL check is being done, else not done.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Lack of minimum deposit can lead to users depositing dust amounts, where a liquidator won't be incentivized to liquidate since gas cost might get more than the amount getting in return, which can increase insolvency in the system.

Proof of Concept

While depositing into any vault, there is no minimum threshold being kept, because of which many users can deposit only dust amounts into their vaults.

Which if comes into position of liquidation, the users won't have any incentive to liquidate , since the gas cost of liquidating might be more expensive than the collateral amount they might get.

Hence , because of this , insolvency into the system can be increased.

Tools Used

Manual reviewing

Recommended Mitigation Steps

Implement a minimum deposit amount threshold while depositing, which would prevent users from depositing dust amount.

Assessed type

Other