DYAD - kennedy1030's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L62-L67 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L67-L78 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L80-L91 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L269-L286 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L250-L267

Vulnerability details

Impact

The total USD value of user's collateral can be calculated to be higher than the actual value, leading to dyad inflation.

Proof of Concept

KerosineManager stores the list of non-kerosene vaults vaultLicenser is intended to store the list of kerosene vaults.

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

        
        KerosineManager kerosineManager = new KerosineManager();

        kerosineManager.add(address(ethVault));
        kerosineManager.add(address(wstEth));

        vaultManager.setKeroseneManager(kerosineManager);

        vaultLicenser.add(address(ethVault));
        vaultLicenser.add(address(wstEth));
        vaultLicenser.add(address(unboundedKerosineVault));
        // vaultLicenser.add(address(boundedKerosineVault));

In VaultManagerV2, however, the vaultsKerosene is utilized to verify the licenses for kerosene vaults.(Line 88, 280)

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

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

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

    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;
L280        if (keroseneManager.isLicensed(address(vault))) {
                usdValue = vault.getUsdValue(id);        
            }
            totalUsdValue += usdValue;
        }
        return totalUsdValue;
    }

So, users have the ability to add their non-kerosene vaults to their vaultsKerosene[id], resulting in the NonKeroseneValue of the given ID being added twice. This increases the user's collateral ratio and leads to inflation of dyad.

Also, because unboundedKerosineVault was added into vaultLicenser in Deploy.V2.s.sol, users can add their kerosineVault to vaults[id]. This may lead to similar issues.

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

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

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

    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;
L261        if (vaultLicenser.isLicensed(address(vault))) {
            usdValue = vault.getUsdValue(id);        
            }
            totalUsdValue += usdValue;
        }
        return totalUsdValue;
    }

Tools Used

Manual review

Recommended Mitigation Steps

A licenser for kerosene vaults should be used in place of KerosineManager within VaultManagerV2 as follows. Additionally, I suggest using a more suitable name for KerosineManager. I believe this name might cause confusion, as KerosineManager actually stores the list of non-kerosene vaults.

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

    [...]

-   KerosineManager public keroseneManager;
+   Licenser public kerosineLicenser;

    [...]

-   function setKeroseneManager(KerosineManager _keroseneManager)
+   function setKeroseneLicenser(Licenser _kerosineLicenser)
        external
        initializer 
        {
-         keroseneManager = _keroseneManager;
+         kerosineLicenser = _kerosineLicenser;  
    }

    [...]

-   if (!keroseneManager.isLicensed(vault))                 revert VaultNotLicensed();
+   if (!kerosineLicenser.isLicensed(vault))                revert VaultNotLicensed();

    [...]

        uint usdValue;
-       if (keroseneManager.isLicensed(address(vault))) {
+       if (kerosineLicenser.isLicensed(address(vault))) {
          usdValue = vault.getUsdValue(id);        
        }

    [...]

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

    
+       Licenser kerosineLicenser = new Licenser();

-       vaultManager.setKeroseneManager(kerosineManager);
+       vaultManager.setKeroseneLicenser(kerosineLicenser); 

+       kerosineLicenser.add(address(unboundedKerosineVault));
+       // kerosineLicenser.add(address(boundedKerosineVault));

+       kerosineLicenser.transferOwnership(MAINNET_OWNER);

-       vaultLicenser.add(address(unboundedKerosineVault));

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L119-L153 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L94-L104 https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L184-L202

Vulnerability details

Impact

Denial of service (DoS) during during withdraw(), redeemDyad() or remove().

Proof of Concept

Anyone can deposit assets for another user's id. However, deposits and withdrawals are prevented within the same block in all vaults.

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

    function deposit(
        uint    id,
        address vault,
        uint    amount
    ) 
        external 
        isValidDNft(id)
    {
@>      idToBlockOfLastDeposit[id] = block.number;
        [...]
    }

    function withdraw(
        uint    id,
        address vault,
        uint    amount,
        address to
    ) 
        public
        isDNftOwner(id)
    {
@>      if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
        [...]
    }

Consider the following scenario:

1. Alice and Bob intend to withdraw some assets.
2. Charlie deposits 1 wei for each of Alice and Bob's IDs in every block (or with front running).
3. As a result, Alice and Bob are unable to complete their withdrawals.
4. Additionally, Charlie has the ability to delay the withdrawals for Alice and Bob at his discretion.

Additionally, redeemDyad or remove() would fail due to the 1 wei remaining in the vault.

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

    function remove(
        uint    id,
        address vault
    ) 
        external
        isDNftOwner(id)
    {
@>      if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();
        if (!vaults[id].remove(vault))     revert VaultNotAdded();
        emit Removed(id, vault);
    }

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

    function redeemDyad(
        uint    id,
        address vault,
        uint    amount,
        address to
    )
        external 
        isDNftOwner(id)
        returns (uint) { 
        dyad.burn(id, msg.sender, amount);
        Vault _vault = Vault(vault);
        uint asset = amount 
                        * (10**(_vault.oracle().decimals() + _vault.asset().decimals())) 
                        / _vault.assetPrice() 
                        / 1e18;
@>      withdraw(id, vault, asset, to);
        emit RedeemDyad(id, vault, amount, to);
        return asset;
    }

Tools Used

Manual review

Recommended Mitigation Steps

Only the owner of the ID should be granted the right to deposit for their own ID.

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

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

1. Kerosene assets cannot be withdrawn.
2. Withdrawal will be unfairly reverted due to the invalid collateral check.

Proof of Concept

When depositing, users can deposit into any vaults.

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

And, getNonKeroseneValue(id) only calcualtes assets in the vaults which are added for the ID and are licensed in vaultLicenser.

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

However, in the withdraw() function, the value of withdrawn assets is deducted regardless of whether the vault is Kerosene, added, or licensed.

    function withdraw(
        uint    id,
        address vault,
        uint    amount,
        address to
    ) 
        public
        isDNftOwner(id)
    {
        if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
        uint dyadMinted = dyad.mintedDyad(address(this), id);
        Vault _vault = Vault(vault);
@>      uint value = amount * _vault.assetPrice() 
                    * 1e18 
@>                  / 10**_vault.oracle().decimals() 
                    / 10**_vault.asset().decimals();
@>      if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
        _vault.withdraw(id, to, amount);
        if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
    }

When the vault is Kerosene, this function will revert because Kerosene Vaults do not have an oracle() function. When the vault is unadded or unlicensed, the deduction is unfair because the asset in the vault was not added to the NonKeroseneValue of the ID.

Tools Used

Manual review

Recommended Mitigation Steps

Only the value of assets withdrawn from the non-Kerosene vaults added for the ID should be deducted in the collateral check.

    function withdraw(
        uint    id,
        address vault,
        uint    amount,
        address to
    ) 
        public
        isDNftOwner(id)
    {
        if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
        uint dyadMinted = dyad.mintedDyad(address(this), id);
        Vault _vault = Vault(vault);

+       uint value;
+       if (vaults[id].contains(vault) && vaultLicenser.isLicensed(address(vault)))
+       {
+           value = amount * _vault.assetPrice()
-           uint value = amount * _vault.assetPrice() 
                    * 1e18 
                    / 10**_vault.oracle().decimals() 
                    / 10**_vault.asset().decimals();
+       }

        if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
        _vault.withdraw(id, to, amount);
        if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
    }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The lack of collateral and the decline of the DYAD value.

Proof of Concept

The value of DYAD is secured by non-Kerosene collateral, while the price of Kerosene assets can be manipulated by depositing a large amount of WETH. So, the USD value of non-Kerosene collateral should be bigger than the amount of DYAD minted. In the liquidate() function, however, only the collateral ratio is checked.

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

This make it possible for malicious users to delay liquidation. It also may lead the lack of collateral and the decline of the DYAD value. The main purpose is to build a stable coin, DYAD. The decline of the DYAD value means the failure of this protocol.

Tools Used

Manual review

Recommended Mitigation Steps

The condition NonKeroseneValue >= mintedDyad should be added in the liquidate() function.

    function liquidate(
        uint id,
        uint to
    ) 
    external 
        isValidDNft(id)
        isValidDNft(to)
    {
        uint cr = collatRatio(id);

-       if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();
+       if (cr >= MIN_COLLATERIZATION_RATIO && getNonKeroseneValue(id) >= dyad.mintedDyad(address(this), id)) 
+               revert NotLiquidatable();
        
        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);
    }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

All functions that call the assetPrice() function will always be reverted when the total collateral value is less than the total supply of DYAD. The collatRatio() will be reverted, which is the main function of this protocol. So liquidation, withdrawal, and redemption will also be reverted.

Proof of Concept

The tvl could become less than dyad.totalSupply() because of the decline of the ether value. If the tvl is less than dyad.totalSupply() in the following code, assetPrice() is reverted.

    function assetPrice() 
        public 
        view 
        override
        returns (uint) {
        uint tvl;
        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());
        }
@>      uint numerator   = tvl - dyad.totalSupply();
        uint denominator = kerosineDenominator.denominator();
        return numerator * 1e8 / denominator;
    }

The functions collatRatio(), liquidate(), withdraw() and redeemDyad() in VaultManagerV2 need the call of assetPrice(). So all of them is reverted when tvl < dyad.totalSupply().

Tools Used

Manual review

Recommended Mitigation Steps

    function assetPrice() 
        public 
        view 
        override
        returns (uint) {
        uint tvl;
        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());
        }
+       if (tvl <= dyad.totalSupply()) return 0;
        uint numerator   = tvl - dyad.totalSupply();
        uint denominator = kerosineDenominator.denominator();
        return numerator * 1e8 / denominator;
    }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Liquidators may take fewer assets than expected during liquidation.

Proof of Concept

The collatRatio of the ID is calculated by dividing the TotalUsdValue of the ID by the total mintedDyad.

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

The TotalUsdValue of the ID is the sum of the NonKeroseneValue and the KeroseneValue for the ID.

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

The NonKeroseneValue is the value of the NonKerosene assets deposited for the given ID. And KeroseneValue is the value of Kerosene assets.

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

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

In VaultManagerV2.liquidate(), however, only Kerosene assets are moved from liquidatee to liquidator.

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

This is unfair to the liquidator. Consider the following scenario:

1. The KeroseneValue of the liquidated ID = 1000, and the NonKeroseneValue = 1000 and mintedDyad = 1600. Then collatRatio = 2000/1600 = 1.25 < 1.5.
2. Now, cappedCr = 2000 / 1600 = 1.25 , liquidationEquityShare = (1.25 - 1) * 0.2 = 0.05, liquidationAssetShare = 1.05
3. The liquidator is supposed to take 1.05/1.25 portion of total value of the ID. That is, 2000 * 1.05 / 1.25 = 1680.
4. However, liquidator only takes the 1.05/1.25 portion of NonKerosene value of the ID. That is, 1000 * 1.05 / 1.25 = 840.

Tools Used

Manual review

Recommended Mitigation Steps

Kerosene assets should be moved during liquidation.

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

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

        emit Liquidate(id, msg.sender, to);
    }

I recommend modifying the liquidate() function to include additional parameters that allow liquidators to choose the ratio between NonKerosene and Kerosene they desire.

Assessed type

Other