Al-Qa-qa / dyad-private-audit

4 stars 1 forks source link

[L-02]: Deposit function is not checking if the vault is licensed or not #2

Closed Al-Qa-qa closed 3 months ago

Al-Qa-qa commented 3 months ago

Description

VaultManagerV3::deposit() allows depositing to any vault even if it is unlicensed. Since a specific vault can get unlicenced at any time, the user can end up depositing funds in a vault that is unlicensed.

Another thing is that the Developer Team can added and removed vaults recently, and there are some upgrades that happened to the vaultManager to mitigate people's funds. So there is a chance that the users can deposit to a vault that got unlicensed by the team recently.

Recommendations

We should check that the vault used to deposit funds is licensed, before making the deposit function.

VaultManagerV3.sol#L86-L98

  function deposit( ... ) external isDNftOwner(id) {
+  require(vaultLicenser.isLicensed(vault), "VaultManagerV3: The vault is not a licenced vault");
    ...
  }

NOTE: It is not recommended to add this in withdraw/redeem functions, as if the vault gets unlicensed, users should be able to withdraw their funds from that unlicensed vault. So if we add this check in withdraw/redeem, users' funds will get locked in the case of unlicensing a vault.

shafu0x commented 3 months ago

this is a user error. we don't check for user errors.

Al-Qa-qa commented 3 months ago

You are right. Although there may be some situations where the likelihood increases, it remains a user error as you said.

In this case, The issue is invalid as it is not from the design choice to check for input user errors.

You can comment on the issue if you want to add something.