DYAD - web3km's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/script/deploy/Deploy.V2.s.sol#L95 https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L75

Vulnerability details

Impact

Malicious user can add UnboundedKeroseneVault to both vaults and vaultsKerosene, causing VaultManagerV2::getTotalUsdValue() to return more than the user actually has, possibly creating bad debt for the protocol.

Proof of Concept

The function VaultManager::add() performs a very crucial check to see if the vault is licensed before adding it.

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

However inside DeployV2.s.sol at line 95, we are licensing the unbounded kerosene vault. Which will cause this check to pass and add the kerosene vault in the list of exogenous vaults.

Paste this inside of test/fork/v2.sol.

function testUserCanAddNonKeroseneVaultsToVaultsList() public {
        DNft dNft = DNft(MAINNET_DNFT);
        address user = makeAddr("user");

        vm.deal(user, 200 ether);
        vm.startPrank(user);
        uint id = dNft.mintNft{value: 5 ether}(user);

        VaultManagerV2 vaultManager = contracts.vaultManager;

        vaultManager.add(id, address(contracts.ethVault));
        vaultManager.add(id, address(contracts.unboundedKerosineVault));

        vm.stopPrank();
    }

and run a forked test using:

forge test --fork-url <MAINNET_FORK_URL> --match-test testUserCanAddNonKeroseneVaultsToVaultsList

Tools Used

Foundry, Manual review

Recommended Mitigation Steps

Consider not licensing any Kerosene vaults through the licenser as the users will be able to add them to their exogenous collateral.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/KerosineManager.sol#L44-L52 https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L88

Vulnerability details

Impact

The current implmentation has 2 problems.

1. Users will be able to add exogenous vault in VaultManagerV2::vaultsKerosene
2. Users will NOT be able to add any kerosene vaults into VaultManagerV2::vaultsKerosene

Proof of Concept

The current role of KerosineManager is to store a list of vaults that will be used for calculating kerosene's usd price.

UnboundedKerosineVault::assetPrice() calls KerosineManager::getVaults() to get the total usd value of the vaults and then calculates kerosene's usd value In line 50-69.

However, the problem occurs with how the KerosineManager::isLicensed(), is used inside of VaultManagerV2::addKerosene().

 function addKerosene(uint id, address vault) external isDNftOwner(id) {
        if (vaultsKerosene[id].length() >= MAX_VAULTS_KEROSENE)
            revert TooManyVaults();
        //@review - `isLicensed` returns true for the vaults used for exogenous collateral instead of the actual kerosene vaults.
        if (!keroseneManager.isLicensed(vault)) revert VaultNotLicensed();
        if (!vaultsKerosene[id].add(vault)) revert VaultAlreadyAdded();
        emit Added(id, vault);
    }

1. Since the isLicensed checks for exogenous vaults, no user will be able to add kerosene as collateral

2. A malicious actor will be able to double the return of VaultManagerV2::getTotalUsdValue() by by adding the same vaults he has in vaults to vaultsKerosene, which can lead to bad debt, since 1/2 of the usd value returned does not exist.

Coded PoC

PoC of user doubling his collateral Paste this chunk of code inside v2.t.sol and in the terminal run

forge test --fork-url <MAINNET_FORK_URL> --match-test testUserCanDoubleHisCollateralByAddingHisVaultToKeroseneSet

Tools Used

Foundry

Recommended Mitigation Steps

Consider adding another storage variable for licensed kerosene vaults and use it within the isLicensed function.

contract KerosineManager is Owned(msg.sender) {
...
+   EnumerableSet.AddressSet private licensedKeroseneVaults;

+   function addKerosene(address vault) external onlyOwner {
+       if (licensedKeroseneVaults.length() >= MAX_VAULTS)
+           revert TooManyVaults();
+       if (!licensedKeroseneVaults.add(vault)) revert VaultAlreadyAdded();
+   }

+   function removeKerosene(address vault) external onlyOwner {
+       if (!licensedKeroseneVaults.remove(vault)) revert VaultNotFound();
+   }
...
    function isLicensed(address vault) external view returns (bool) {
-       return vaults.contains(vault);
+       return licensedKeroseneVaults.contains(vault);  
    }
}

Assessed type

Other

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#L94-L104 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L106-L116

Vulnerability details

Impact

Users will not be able to remove their vault, because of malicious actor depositing as much as 1 wei before they remove it.

Proof of Concept

Within the VaultManagerV2::deposit(), we are only checking if the id of the note sent is valid.

    function deposit(
        uint id,
        address vault,
        uint amount
    ) external isValidDNft(id) { //@review - Anyone can deposit into any note
        Vault _vault = Vault(vault);
        _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
        _vault.deposit(id, amount);
    }

A malicious actor can monitor the mempool and wait for a user to execute VaultManagerV2::remove() or VaultManagerV2::removeKerosene. The griefer can front-run the transaction and deposit 1 wei into the vault the user wants to remove. Making his transaction revert due to check in VaultManagerV2::remove() and VaultManagerV2::removeKerosene

  function remove(uint id, address vault) external isDNftOwner(id) {
@>      if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets(); //@review - Since the griefer deposited 1 wei into his vault the function will revert
        if (!vaults[id].remove(vault)) revert VaultNotAdded();
        emit Removed(id, vault);
    }

 function removeKerosene(uint id, address vault) external isDNftOwner(id) {
 @>     if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets(); //@review - Since the griefer deposited 1 wei into his vault the function will revert
        if (!vaultsKerosene[id].remove(vault)) revert VaultNotAdded();
        emit Removed(id, vault);
    }

Now the user will have to withdraw the remaining 1 wei, before attempting to remove the vault again.

Tools Used

Foundry, Manual review

Recommended Mitigation Steps

Consider only allowing the owner of the note to deposit into it.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L146-L149 https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L195-L198

Vulnerability details

Impact

Users will not be able to withdraw or reedem for Kerosene vaults.

Proof of Concept

Both functions VaultManagerV2::withdraw() and VaultManagerV2::redeemDyad() denominate a value based on the decimals of the vault's oracle.

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

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

However, after the upgrade there are 2 new vaults, Bounded and unbounded kerosene vaults. One of the main differences between the two vaults is that the Kerosene Vault does not implement an oracle to get its value, but is caclulated deterministically depending on the total tvl and the total supply of dyad.

When any of the functions that depend on the vault's oracle are called for any kerosene vault they will inevitably revert.

Coded PoC of Unbounded Kerosene vault reverting when trying to call oracle method Please run the coded poc with:

forge test --fork-url <MAINET_FORK_URL> --match-test testKeroseneVaultsDontHaveOracle  -vv 

Recommended Mitigation Steps

Consider adding some type of storage variable/function to get Kerosene vault's price decimals, then a helper function for the manager to exctract the decimals from the vault.

abstract contract KerosineVault is IVault, Owned(msg.sender) {
    using SafeTransferLib for ERC20;

    IVaultManager public immutable vaultManager;
    ERC20 public immutable asset;
    KerosineManager public immutable kerosineManager;

+   uint8 public decimals = 8;

   ...
}

function _vaultPriceDecimals(
        address vault,
        uint256 id
    ) internal view returns (uint256) {
        return
            vaults[id].contains(vault)
                ? Vault(vault).oracle().decimals()
                : KerosineVault(vault).decimals();
    }

    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 priceDecimals = _vaultPriceDecimals(address(_vault), id);

-       uint asset = (amount *
-         (10 ** (_vault.oracle().decimals() + _vault.asset().decimals()))) /
-           _vault.assetPrice() /
-           1e18;
+       uint asset = (amount *
+           (10 ** (priceDecimals + _vault.asset().decimals()))) /
+           _vault.assetPrice() /
+           1e18;


        withdraw(id, vault, asset, to);
        emit RedeemDyad(id, vault, amount, to);
        return asset;
    }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

1. Early adopter can manipulate prices in order to liquidate positions that depend on kerosene, and possibly receiving the collateral.

2. Users who have added any kerosene vault to their note, will not be able to mint/redeem/withdraw until the TVL of the new vaults deployed exceeds the total supply of DYAD.

Proof of Concept

The team intents to deploy two separate vaults for V2. However since dyad as of the time writting has ~ 632,967 total Supply, but the TVL of the V2 system will initially be 0, causing UnboundedKerosineVault::assetPrice() to revert due to underflow.

2 issues can occur by having a completely new tvl for the new system.

1. Since initally the supply will be far greater than the tvl, any user that has added a kerosene vault to his note id, will not be able to mint, redeem, nor get liquidated, because of VaultManagerV2::collatRatio() always reverting.

A malicious actor can take advantage of this and intentionally add a kerosene vault to his vaultsKerosene to make sure he can never get liquidated, which can account for bad debt.

   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(); //@review -> Total supply will initially be far greater than the tvl of the V2 system.
        uint denominator = kerosineDenominator.denominator();
        return (numerator * 1e8) / denominator;
    }

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

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

    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); //@review - If the note passed, has Unbounded or bounded kerosene vaults added this function will revert due to underflow
            }
            totalUsdValue += usdValue;
        }
        return totalUsdValue;
    }

2. Upon launching a malicious actor can deposit more $collateral than the total supply of dyad without minting any. After some time, when the system receives more collateral, and users start to use Kerosene to cover some of their overcollateralization, the malicious actor can start withdrawing small pieces of his collateral to lower Kerosene's price, get users liquidated and then deposit again.

Marking the issue as Medium, as the team can act quick and add the old vaults to KerosineManager::vaults, to pump the tvl over the total supply.

Tools Used

Foundry, Manual Analysis

Recommended Mitigation Steps

Consider adding the V1 vaults to KerosineManager::vaults.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The protocol can be undercollateralized potentially, which breaks the invariant tvl > totalSupply of dyad, which leads to UnboundedKerosineVault::assetPrice() always reverting.

Proof of Concept

Liquidators liquidate users to turn a profit, but it becomes tricky when the value is too low. For example, if an account has only $5.50 worth of collateral and owes 4 DYAD, it's undercollateralized and needs liquidation to keep the protocol safe. However, with such a small value, after considering gas costs, liquidators may not make any profit. Consequently, these low-value accounts might never get liquidated, leaving the protocol with bad debt and risking undercollateralization.

If too many of those small positions never get liquidated users that have added UnboundedKeroseneVault to their vaultsKerosene will never be able to mint/redeem/withdraw as UnboundedKerosineVault::assetPrice() will always revert.

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

            //@note - Seems correct
            tvl +=
                (vault.asset().balanceOf(address(vault)) * //@audit - balanceOf() can be manipulatable
                    vault.assetPrice() *
                    1e18) /
                (10 ** vault.asset().decimals()) /
                (10 ** vault.oracle().decimals());
        }
        
        // @review - tvl will be smaller than total supply, if too many small positions do not get liquidated.
        uint numerator = tvl - dyad.totalSupply();
        uint denominator = kerosineDenominator.denominator();
        return (numerator * 1e8) / denominator;
    }

Tools Used

Foundry, Manual review

Recommended Mitigation Steps

Consider only allowing users to mint Dyad if their collateral value is past a certain threshold.

Assessed type

DoS