DYAD - grearlake's results

Lines of code

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

Vulnerability details

Vulnerability details

From run() function, it can be seen that ethVault and wstEth are added to both kerosineManager and vaultLicenser:

kerosineManager.add(address(ethVault));  // <--
kerosineManager.add(address(wstEth));    // <--

.  .  .  .  .  .

vaultLicenser.add(address(ethVault));    // <--
vaultLicenser.add(address(wstEth));      // <--
vaultLicenser.add(address(unboundedKerosineVault));

In VaultManagerV2() contract, to calculate valuable of dNFT, getTotalUsdValue() function is used:

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

Function getNonKeroseneValue() get value of token that added in vaultLicenser contract:

function getNonKeroseneValue(uint id) public view returns (uint) {
    uint totalUsdValue;
    uint numberOfVaults = vaults[id].length(); 
    for (uint i = 0; i < numberOfVaults; i++) {
        Vault vault = Vault(vaults[id].at(i));
        uint usdValue;
        if (vaultLicenser.isLicensed(address(vault))) {  // <---
            usdValue = vault.getUsdValue(id);    // <---
        }
        totalUsdValue += usdValue;
    }
    return totalUsdValue;
}

Similar with it, getKeroseneValue() get value of token that added in keroseneManager contract:

function getKeroseneValue(uint id) public view returns (uint) {
    uint totalUsdValue;
    uint numberOfVaults = vaultsKerosene[id].length(); 
    for (uint i = 0; i < numberOfVaults; i++) {
        Vault vault = Vault(vaultsKerosene[id].at(i));
        uint usdValue;
        if (keroseneManager.isLicensed(address(vault))) {  // <---
            usdValue = vault.getUsdValue(id);  // <---
        }
        totalUsdValue += usdValue;
    }
    return totalUsdValue;
}

Since the WETH and WstETH added 2 time, it will be counted twice, which will return incorrect price

Impact

Price will be incorrectly returned,

Tools Used

Manual review

Recommended Mitigation Steps

Only add WETH and WstETH vault once.

Assessed type

Other

Lines of code

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

Vulnerability details

Vulnerability details

In withdraw() function, there is a checking condition to restrict user from withdraw token in the same block to mitigrating flashloan attack:

  if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();

But anyone can deposit to id's dNFT vault, and idToBlockOfLastDeposit variable is updated in that time:

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

It will open attack scenario that attacker can front-running, deposit dust amount to victim's id in the same block that victim withdraw, make withdraw's transaction reverted.

Impact

Victim are not able to withdraw in that block

Tools Used

Manual review

Recommended Mitigation Steps

Add reentrancy protection to mitigrating flashloan attack instead.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L217-#L219

Vulnerability details

Vulnerability details

In liquidate() function, rate is calculated as below:

  uint cappedCr               = cr < 1e18 ? 1e18 : cr;
  uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD);
  uint liquidationAssetShare  = (liquidationEquityShare + 1e18).divWadDown(cappedCr);

With cr = 1.2e18 => liquidationEquityShare = (1.2e18 - 1e18) * 0.2e18 / 1e18 = 0.06e18 => liquidationAssetShare = (0.06e18 + 1e18) * 1e18 / 1.3e18 = 0.815e18, which is not correct compare to document: The liquidator burns a quantity of DYAD equal to the target Note’s DYAD minted balance, and in return receives an equivalent value plus a 20% bonus of the target Note’s backing colateral. From that example, liquidator should receive (1e18 + 0.2e18 * 20%) / 1.2 = 0.86667e18 = 86,67% of the dNFT's total token, and protocol keep 13,33% of the last one.

Impact

Wrong number of token is sent to liquidator, they receive less token than they should

Tools Used

Manual review

Recommended Mitigation Steps

Update formula to below:

  uint cappedCr               = cr <= 1e18 ? 1e18 : cr;
  uint liquidationAssetShare  = cr <= 1e18 ? 1e18 : (1e18 + (cr - 1e18).mulWadDown(LIQUIDATION_REWARD)).divWadDown(cappedCr)
  uint liquidationEquityShare = cr <= 1e18 ? 0 : 1e18 - liquidationAssetShare

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L205-#L228

Vulnerability details

Vulnerability details

Function liquidate() is used to liquidate dNFT that collateral ratio is under 150%:

function liquidate(uint id, uint to) external 
    isValidDNft(id)
    isValidDNft(to)
{
    uint cr = collatRatio(id);
    if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();
    dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id));

    uint cappedCr               = cr < 1e18 ? 1e18 : cr;
    uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD);
    uint liquidationAssetShare  = (liquidationEquityShare + 1e18).divWadDown(cappedCr);

    uint numberOfVaults = vaults[id].length();
    for (uint i = 0; i < numberOfVaults; i++) {
        Vault vault      = Vault(vaults[id].at(i));
        uint  collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare);
        vault.move(id, to, collateral);
    }
    emit Liquidate(id, msg.sender, to);
}

When user liquidate vault, they will receive 20% bonus of the target Note’s backing colateral. But in the contract, there is no mechanism to restrict minimum amount to deposit/minting. Which will open the attack vector that user will mint very small amount of dyad token, and when that dNFT is underwater, there is no incentive for user to liquidate them because the cost to liquidating is higher than number of gas required to call liquidate() function, which will lead to bad debt for protocol

Impact

dNFT that under-collateralized is no liquidated, which will lead to bad debt for protocol, and dyad token lost some backup token

Tools Used

Manual review

Recommended Mitigation Steps

Add minimum number of token should be used to be able to mint dyad token.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L101 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L113

Vulnerability details

Vulnerability details

In both remove() and removeKerosine() function, there is a checking condition:

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

But function deposit() only check if id is valid or not, but not id is belong to caller:

function deposit(uint id, address vault, uint amount) external 
    isValidDNft(id)    // <----
{
    idToBlockOfLastDeposit[id] = block.number;
    Vault _vault = Vault(vault);
    _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
    _vault.deposit(id, amount);
}

It will lead to scenario that attacker will front-running and deposit dust amount for victim's id, make them are not able to remove vault

Impact

Attacker can make user failed from removing vault of their DNFT

Tools Used

Manual review

Recommended Mitigation Steps

Only allow owner to deposit to that dNFT

function deposit(uint id,address vault,uint amount) external 
   -   isValidDNft(id)
   +   isDNftOwner(id)

Assessed type

Other