DYAD - poslednaya's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L64

Vulnerability details

Impact

User can double deposit via providing the same Vault address to both Non-Kerosene and Kerosene Vaults lists if protocol will be deployed via provided Deploy.V2 script.

Proof of Concept

Protocol has 2 licensing system to approve vaults addresses.

To approve non-Kerosene Vaults there is Licencer-instance that deployed with VaultManagerV2.

To approve Kerosene Vaults there is KerosineManager.

In Deploy.V2.s.sol you add ethVault and wstEth to KerosineVaults. And also then you will licensing these vaults via VaultManagerV2 licenser.

In this case user can add same Vault to both vaults and keroseneVaults. And deposit will double when accounting via getTotalUsdValue because same vault will be accounted in both Kerosene/NonKerosene vaults.

// Deploy.V2.s.sol
...
    kerosineManager.add(address(ethVault));
    kerosineManager.add(address(wstEth));
...

// KerosineManager
...
    EnumerableSet.AddressSet private vaults;
...

// VaultManagerV2
 function add(uint id, address vault) external isDNftOwner(id) {
    ...
    if (!vaultLicenser.isLicensed(vault))  revert VaultNotLicensed(); // passed
    ...
  }

 function addKerosene(uint id, address vault) external isDNftOwner(id) {
    ...
    if (!keroseneManager.isLicensed(vault)) revert VaultNotLicensed(); // passed
    ...
  }

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

keroseneManager: https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L64

vaultLicenser: https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L95

Tools Used

Manual review.

Recommended Mitigation Steps

Be care with mixing when approve Non-Kerosene and Kerosene Vaults. Don't add ethVault and wstEth to KeroseneManager, they should be added to VaultManagerV2 Licenser-instance. And don't add Kerosene Vaults to vaultLicenser.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

In VaultManager::withdraw() there is check idToBlockOfLastDeposit[id] == block.number to prevent deposit/withdraw in the same block. Malicious user can frontrun user withdrawal transaction with his VaultManager::deposit() and pass id of user's NFT and amount eq 1 Wei. So the user's withdraw transaction will revert because check for block.number will not passing.

Because there is no minimum amount of deposit and protocol allows anyone to deposit to any Vault (isValidDNft(id) doesn't check msg.sender)- malicious actor can DoS any Vault at any time, including the administrator's Vaults.

Proof of Concept

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

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

Tools Used

Manual review.

Recommended Mitigation Steps

Consider to add minimum amount of deposit to make this attack unprofitable for attacker and profitable to Vault owner.

Assessed type

DoS