DYAD - sil3th's results

Lines of code

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

Vulnerability details

Impact

Vaults will be unable to be removed since they need not to have assets on removal.

Proof of Concept

• Bob a malicious user adds an invalidDNFT in the vault since the add function does not check whether the NFT is valid or not.

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

• When trying to remove the vault using the remove function it will check if the vaultHasAssets as shown in the check below from the remove function making it impossible to remove the vault.

 
    if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();
  

Tools Used

Manual Review

Recommended Mitigation Steps

Add a check on the add function to ensure only valid NFTs can be added to a vault. As shown below:

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

Assessed type

DoS