DYAD - imare's results

Lines of code

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

Vulnerability details

Impact

VaultManagerV2 deploys a protection against flashloans by not allowing a Note owner from depositing and withdrawing in the same block. But this same protection allows anyone else to DoS a honest Note owner from withdrawing.

Proof of Concept

As it can be observed from the deploy script VaultManager currently users can use/add the following exo/non-kerosine Vaults:

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

Vaults allows for anyone to do zero amount deposits:

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

Both assets in the used Vaults can be deposit with zero amounts without reverting the transaction):

https://etherscan.io/address/0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2#code#L63 https://etherscan.io/address/0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0#code#L479

The following POC demonstrated this zero amount deposit (add the following functions and imports to v2.t.sol file):

import {DNft}                   from "../../src/core/DNft.sol";
import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";

contract V2Test is Test, Parameters, IERC721Receiver {
...

function testZeroVaultDeposit() public {
    DNft note = DNft(MAINNET_DNFT);
    uint priceToMintNote = note.START_PRICE() + (note.PRICE_INCREASE() * note.publicMints());
    console.log("price", priceToMintNote);

    uint noteId = note.mintNft{value : priceToMintNote}(address(this));

    // Note : Note owner can add Vault later. Deposits into the Vault will work nevertheless
    contracts.vaultManager.deposit(noteId, address(contracts.wstEth), 0);
    contracts.vaultManager.deposit(noteId, address(contracts.ethVault), 0);
  }

  function onERC721Received(
      address,
      address,
      uint256,
      bytes memory
  ) external override returns (bytes4) {
      return IERC721Receiver.onERC721Received.selector;
  

It can be observed that the test passes without reverting:

Ran 1 test for test/fork/v2.t.sol:V2Test
[PASS] testZeroVaultDeposit() (gas: 225846)

Note: this kind of the deposit can be done by ayone becouse the deposit function checks only for the Note token to exists with isValidDNft

The consequence of this zero deposit in the same block is that the owner cannot withdraw/reedemDyad and is effectively DoS-ed by anyone.

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

Tools Used

Manual review

Recommended Mitigation Steps

Require that Vault deposit trough VaultManagerV2 are non zero (or some minimal amount per used Vault asset):

  function deposit(
    uint    id,
    address vault,
    uint    amount
  ) 
    external 
      isValidDNft(id)
  {
+   require(amount >0, "amount zero");
    idToBlockOfLastDeposit[id] = block.number;
    Vault _vault = Vault(vault);
    _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
    _vault.deposit(id, amount);
  }

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L48-L65 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Vault.kerosine.unbounded.sol#L56-L64 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Vault.kerosine.unbounded.sol#L65 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManager.sol#L101-L112 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Vault.kerosine.bounded.sol#L44-L51

Vulnerability details

Impact

Kerosene bounded or unbounded assetPrice() call is an integral part of the protocol. Is mainly used whenever a Note token id owner wants to use a kerosene type of Vault as part of the chosen set of Vaults.

But the call to Kerosene bounded or unbounded assetPrice() is going to revert becouse the current calculation resides on a value that is currently "locked" inside the V1 vaults and not in the newer one.

Proof of Concept

As it can be observed inside the deploy script:

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

the newer Vaults are used in the calculation of the assetPrice call inside the Kerosine.unbounded contract :

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Vault.kerosine.unbounded.sol#L56-L64

where the total balance of token asset per vault is subtracting from the Dyad total supply :

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Vault.kerosine.unbounded.sol#L65

But this line is going to revert becouse the TVL for this newer Vaults is zero (or near zero) and when subtracting will revert as demonstrated in the next POC (add this test to v2.t.sol test file):

function testKeroseneAssertPrice() public {
    contracts.unboundedKerosineVault.assetPrice();
  }

When run will result in a revert :

[FAIL. Reason: panic: arithmetic underflow or overflow (0x11)] testKeroseneAssertPrice() (gas: 138867)
Suite result: FAILED. 

The value of dyad.totalSupply(); that is used in the calculation currently equals to 632967400000000000000000 which means that Dyad token has been already used/minted for the previous V1 version of VaultManager and its accompanying vaults. As it can be seen on this lines from the previous VaultManager:

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManager.sol#L101-L112

Note: when Kerosine.unbounded#assetPrice() reverts it also means that Kerosine.bounded#assetPrice() will revert too becouse it calls on the previous one:

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/Vault.kerosine.bounded.sol#L44-L51

Tools Used

Manual review

Recommended Mitigation Steps

This is a difficult one to mitigate. There should be a migration strategy in place to reutilize the already minted Dyad token or one solution could be to instead of relaying on the total supply of already minted Dyad, instead have per Vault value of minted Dyad. Something like :

function assetPrice() 
    public 
    view 
    override
    returns (uint) {
      uint tvl, dyad;
      address[] memory vaults = kerosineManager.getVaults();
      uint numberOfVaults = vaults.length;
      for (uint i = 0; i < numberOfVaults; i++) {
        Vault vault = Vault(vaults[i]);

        tvl += vault.asset().balanceOf(address(vault)) 
                * vault.assetPrice() * 1e18
                / (10**vault.asset().decimals()) 
                / (10**vault.oracle().decimals());

        dyad += vault.lockedDyad(); // @<-
      }
      console.log("tvl", tvl);
      console.log("totalSupply", dyad.totalSupply());
      uint numerator   = tvl - dyad; // @<-
      ...
      

Looking at the previous VaultManager the V1 there is no obvious way of migrate already locked assets to the newer vaults. Also Dyad cannot be just transferred to a new place by a master owner.

Assessed type

Error