Missing vault address validation in the `VaultManagerV2` for `deposit()` and `withdraw()`, exposes protocol to risk of reentrancy, unfair liquidation and phishing

[c4-bot-6]

Lines of code

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

Vulnerability details

Impact

There is no address validation for the vault parameter in the VaultManagerV2, for the vault address in the deposit()and withdraw() which exposes the protocol to different set of risks:

• Users could supply the address of malicious contract that implements the IVault interface and make the vaults make calls to it openeing an attack surface for reentrancy attacks: we couldn't find any way of exploiting this to do some damage. But we still recommend closing unnecessary attack surfaces and avoiding external calls to user given adresses
• If a user deposits his fund to a vault that he didn't add in the VaultManagerV2 through the add() or addKerosene(). The funds will still be deposited in the vault but they wouldn't be counted in when calculating the collateralisation ratio of his position, as the function will only iterate through the vaults the user adds and potentially this could lead to unfair liquidations.
• The VaultManagerV2 could be a target of fishing campaings by other Attackers. The VaultManagerV2 will be first not flagged and reported by web3 wallet like metamask. So the attackers will make the victims call deposit with their funds on address that belongs to a malicous contract the attacker deployed. So the user will deposit his funds to the VaultManagerV2 but his funds will get deposited to the malicious contract and the attacker will get the funds from the malicious vault.

  Recomendation

  To mitigate this attack vector, and protect both the users and the protocol from attack risks. We recommed that in the deposit() and withdraw() functions, that the protocol checks if the given vault address is an address that the user has already added else revert
  A possible fix would like this:

  
  function deposit(
  uint    id,
  address vault,
  uint    amount
  ) 
  external 
    isValidDNft(id)
  {
  idToBlockOfLastDeposit[id] = block.number;

• if (!(vaults[id].contains(vault) || vaultsKerosene[id].contains(vault))) revert NotAdded(); Vault _vault = Vault(vault); _vault.asset().safeTransferFrom(msg.sender, address(vault), amount); _vault.deposit(id, amount); }

```diff
function withdraw(
    uint    id,
    address vault,
    uint    amount,
    address to
  ) 
    public
      isDNftOwner(id)
  {
    if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
+    if (!(vaults[id].contains(vault) || vaultsKerosene[id].contains(vault))) revert NotAdded();
    uint dyadMinted = dyad.mintedDyad(address(this), id);
    Vault _vault = Vault(vault);

Assessed type

Invalid Validation

[c4-pre-sort]

JustDravee marked the issue as duplicate of #555

[c4-pre-sort]

JustDravee marked the issue as sufficient quality report

[c4-judge]

koolexcrypto marked the issue as unsatisfactory: Invalid