DYAD - shaflow2's results

Lines of code

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

Vulnerability details

Impact

The system is designed with the vaultsKerosene mapping storing vaultsKerosene, and vaults storing other vaults (currently only WETH and wstETH).

However, incorrect implementation of the add and remove functions in the VaultManagerV2 contract results in the system deviating from its design.

From Deploy.V2.s.sol, it can be seen that all vaults will be added to the Licenser contract. The KerosineManager contract is used to store regular vaults used for LVT valuation (WETH and wstETH).

However, in the VaultManagerV2 contract, it mistakenly assumes that WETH and wstETH vaults will be added to the Licenser contract, and vaultsKerosene will be added to the KerosineManager contract. GitHub:https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L67

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

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

This leads to the following consequences:

1. Users can add vaultsKerosene to the vaults mapping, meaning Kerosine will also participate in the calculation of the value of the getNonKeroseneValue function.
2. Users can repeatedly add regular vaults used for valuation in the KerosineManager contract to vaultsKerosene, causing them to be double-counted in value calculation.

Proof of Concept

Below is part of the test code:

    function testValutAddError() public {
        testLicenseVaultManager();

        address addr1 = makeAddr("addr1");

        deal(MAINNET_WETH, addr1, 10e18);
        assertEq(IERC20(MAINNET_WETH).balanceOf(addr1), 10e18);

        vm.startPrank(MAINNET_OWNER);
        uint256 id1 = DNft(MAINNET_DNFT).mintInsiderNft(addr1);
        IERC20(MAINNET_KEROSENE).transfer(addr1, 10e18);
        assertEq(IERC20(MAINNET_KEROSENE).balanceOf(addr1), 10e18);
        vm.stopPrank();

        vm.startPrank(addr1);
        //deposit weth
        contracts.vaultManager.add(id1, address(contracts.ethVault));
        IERC20(MAINNET_WETH).approve(address(contracts.vaultManager), 1e18);

        contracts.vaultManager.deposit(id1, address(contracts.ethVault), 1e18);
        console.log("AfterDepositInWETHVault noKerosineVaultValue:");
        console.log(contracts.vaultManager.getNonKeroseneValue(id1));

        //repeat add weth
        console.log("Before repeat add wethVault totalValue:");
        console.log(contracts.vaultManager.getTotalUsdValue(id1));
        contracts.vaultManager.addKerosene(id1, address(contracts.ethVault));
        console.log("After repeat add wethVault totalValue:");
        console.log(contracts.vaultManager.getTotalUsdValue(id1));
        vm.stopPrank();
    }

Tools Used

Manual audit, foundry

Recommended Mitigation Steps

Add a mapping in the licenser contract to store the Kerosine vaults:

contract Licenser is Owned(msg.sender) {
    mapping(address => bool) public isLicensed;
    mapping(address => bool) public isKerosineLicensed;
    constructor() {}

    function add(address vault) external onlyOwner {
        isLicensed[vault] = true;
    }
    function addKerosine(address vault) external onlyOwner {
        isKerosineLicensed[vault] = true;
    }
    function remove(address vault) external onlyOwner {
        isLicensed[vault] = false;
    }
    function removeKerosine(address vault) external onlyOwner {
        isKerosineLicensed[vault] = false;
    }
}

VaultManager Contract

    function addKerosene(uint id, address vault) external isDNftOwner(id) {
        if (vaultsKerosene[id].length() >= MAX_VAULTS_KEROSENE)
            revert TooManyVaults();
        if (!vaultLicenser.isKerosineLicensed(vault)) revert VaultNotLicensed();
        if (!vaultsKerosene[id].add(vault)) revert VaultAlreadyAdded();
        emit Added(id, vault);
    }
    function removeKerosene(uint id, address vault) external isDNftOwner(id) {
        if (Vault(vault).id2asset(id) > 0) revert VaultHasAssets();
        if (!vaultLicenser.isKerosineLicensed(vault)) revert VaultNotLicensed();
        emit Removed(id, vault);
    }

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Attackers can monitor the txpool, front-run based on MEV, and perform DoS on a withdrawal operation of a specific NFT user.

Proof of Concept

Since anyone can deposit for an NFT, regardless of whether they are the owner of the NFT.
However, the contract specifies that deposit and withdrawal operations for an NFT cannot occur in the same block.
github:https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L143

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

There is no limit on the amount of deposit, which means the attacker only needs to spend gas to perform a DoS attack on the target NFT.

POC:

1. Alice mints an NFT and deposits 100 WETH into VaultManager.
2. Now Alice wants to withdraw some WETH.
3. Bob, a malicious attacker, monitors Alice's withdraw transaction in the txpool, performs transaction front-running, and executes one or more (to increase success rate) deposit transactions with parameters being Alice's NFT id, a randomly chosen vault address, amount=0, before Alice's transaction is included.
4. Since Bob's transaction gets included in the mempool ahead of Alice's, Alice's transaction might revert.

Tools Used

Manual audit

Recommended Mitigation Steps

1. Control that only the owner of the NFT can perform deposit operations.

function deposit(
    uint id,
    address vault,
    uint amount
) external isDNftOwner(id) {

2. Set a minimum deposit amount to increase the cost of attacks.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

The design of UnboundedKerosineVault allows users to deposit and withdraw Kerosine through VaultManagerV2. In the implementation of VaultManagerV2, users can deposit Kerosine into UnboundedKerosineVault by calling the deposit method in VaultManagerV2. However, when attempting to withdraw through a call to withdraw, the transaction will revert.

Proof of Concept

The reason for this is that Vault and KerosineVault have different valuation methods, and VaultManagerV2 does not consider the valuation method of KerosineVault.
Vault obtains prices through an oracle, hence it has an oracle field.
However, the withdraw function calls the oracle to get the price decimals, and since KerosineVault does not have an oracle field, it reverts during withdrawal.
github:https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L146C1-L150C1

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

POC: Create test functions in v2.t.sol.

    function testKerosineWithdraw() public {
        testLicenseVaultManager();

        address addr1 = makeAddr("addr1");

        deal(MAINNET_KEROSENE, addr1, 100e18);
        assertEq(IERC20(MAINNET_KEROSENE).balanceOf(addr1), 100e18);
        vm.startPrank(MAINNET_OWNER);
        uint id1 = DNft(MAINNET_DNFT).mintInsiderNft(addr1); //id 645
        vm.stopPrank();

        vm.startPrank(addr1);
        IERC20(MAINNET_KEROSENE).approve(
            address(contracts.vaultManager),
            100e18
        );
        vm.roll(1);
        contracts.vaultManager.deposit(
            id1,
            address(contracts.unboundedKerosineVault),
            100e18
        );
        vm.roll(2);
        vm.expectRevert();
        contracts.vaultManager.withdraw(
            id1,
            address(contracts.unboundedKerosineVault),
            100e18,
            addr1
        );
        vm.stopPrank();
    }

Tools Used

Manual audit, foundry

Recommended Mitigation Steps

    function withdraw(
        uint id,
        address vault,
        uint amount,
        address to
    ) public isDNftOwner(id) {
        if (idToBlockOfLastDeposit[id] == block.number)
            revert DepositedInSameBlock();
+         if (vaultsKerosene[id].contains(vault)) {
            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();
    }

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

During the system migration using the DeployV2 script, the previously deployed Dyad contract is used. Violating the condition LVT > Dyad TotalSupply can lead to a system DoS. Github:https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L42

        VaultManagerV2 vaultManager = new VaultManagerV2(
            DNft(MAINNET_DNFT),
            Dyad(MAINNET_DYAD),
            vaultLicenser
        );

The main reason for this issue is that the Dyad contract on the mainnet already has a total supply of 632967400000000000000000.
However, in V2, each vault is redeployed, resulting in LVT being set to 0. If the old Dyad contract is used to migrate to V2, the system will underflow when calculating the assertPrice in the UnboundedKerosineVault contract.
This will cause users who have deposited into the UnboundedKerosineVault and users who attempt to interact with it to be unable to perform any operations.

        uint numerator = tvl - dyad.totalSupply();

Proof of Concept

Below is the test function I added in v2.t.sol:

    function testgetNonKeroseneValueRevert() public {
        testLicenseVaultManager();

        address addr1 = makeAddr("addr1");

        deal(MAINNET_WETH, addr1, 10e18);
        assertEq(IERC20(MAINNET_WETH).balanceOf(addr1), 10e18);

        vm.startPrank(MAINNET_OWNER);
        uint256 id1 = DNft(MAINNET_DNFT).mintInsiderNft(addr1);
        IERC20(MAINNET_KEROSENE).transfer(addr1, 10e18);
        assertEq(IERC20(MAINNET_KEROSENE).balanceOf(addr1), 10e18);
        vm.stopPrank();
        vm.startPrank(addr1);
        contracts.vaultManager.add(
            id1,
            address(contracts.unboundedKerosineVault)
        );
        IERC20(MAINNET_KEROSENE).approve(address(contracts.vaultManager), 1e18);

        contracts.vaultManager.deposit(
            id1,
            address(contracts.unboundedKerosineVault),
            1e18
        );
        vm.expectRevert();
        contracts.vaultManager.getNonKeroseneValue(id1);
        vm.stopPrank();
    }

Tools Used

Manual audit

Recommended Mitigation Steps

Deploy a new Dyad contract during migration.

Assessed type

DoS