DYAD - tchkvsky's results

Lines of code

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

Vulnerability details

ISSUE

When users want to associate their DNFT with a vault, they will either call VaultManager.add() to add to normal vault or VaultManager.addKerosene() to add to a kerosene vault. On calling the addKerosene(), the function checks if the particular vault is licensed.

    // VaultManager.addKerosene
    function addKerosene(uint256 id, address vault) external isDNftOwner(id) {
        if (vaultsKerosene[id].length() >= MAX_VAULTS_KEROSENE) revert TooManyVaults();
        if (!keroseneManager.isLicensed(vault)) revert VaultNotLicensed();  // <--- Check is done here
        if (!vaultsKerosene[id].add(vault)) revert VaultAlreadyAdded();
        emit Added(id, vault);
    }

Normally, this check is supposed to be done by the Licenser contract (through the multisig) but in this function the check is carried out by the KeroseneManager contract.

    // ...
        if (!keroseneManager.isLicensed(vault)) revert VaultNotLicensed();
    // ...

The KeroseneManager is used to add/remove the vaults of exogenous collateral tokens (WETH/WSTETH) when calculating the asset price of Kerosene. In the deployment script, the Kerosene Manager adds the two "normal" vaults while the Licenser licenses the unbounded kerosene vault.

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

The issue is that when a user wants to associate their DNFT with a kerosene vault (unbounded type in this case), the Vault Manager contract checks and calls KeroseneManager.isLicensed() and the contract cannot find the unbounded kerosene vault in the set. This causes a revert in the addKerosene() call and users cannot associate a DNFT with the kerosene vault.

POC

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

import "forge-std/console.sol";
import {StdUtils} from "lib/forge-std/src/StdUtils.sol";
import "forge-std/Test.sol";
import {IERC20} from "lib/forge-std/src/interfaces/IERC20.sol";

import {DeployV2, Contracts} from "../../script/deploy/Deploy.V2.s.sol";
import {Licenser} from "../../src/core/Licenser.sol";
import {Parameters} from "../../src/params/Parameters.sol";
import {IDNft} from "src/interfaces/IDNft.sol";

contract V2Test is StdUtils, Test, Parameters {
    Contracts contracts;

    IDNft dnft = IDNft(MAINNET_DNFT);

    address payable weth = payable(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);
    address payable wsteth =
        payable(0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0);

    address[] users;

    address payable user1 = payable(makeAddr("user1"));
    address payable user2 = payable(makeAddr("user2"));
    address payable user3 = payable(makeAddr("user3"));
    address payable user4 = payable(makeAddr("user4"));
    address payable user5 = payable(makeAddr("user5"));

    function setUp() public {
        vm.createSelectFork("https://rpc.ankr.com/eth", 19723800);
        contracts = new DeployV2().run(); // Contracts are deployed

        vm.prank(address(MAINNET_OWNER));
        contracts.vaultLicenser.add(address(contracts.vaultManager));
        contracts.vaultLicenser.isLicensed(address(contracts.vaultManager));

        users.push(user1); //id: 645
        users.push(user2);
        users.push(user3);
        users.push(user4);
        users.push(user5);

        _dealUsersTokens();

        IERC20(weth).balanceOf(users[0]);

        _mintDNFT();
    }

    function testIssue() public {
        _depositWithdrawKeroseneVault();
    }

    function _mintDNFT() internal {
        for (uint256 i = 0; i < users.length; i++) {
            vm.prank(address(users[i]));
            dnft.mintNft{value: 0.64 ether}(address(users[i]));
        }
    }

    function _dealUsersTokens() internal {
        for (uint256 i = 0; i < users.length; i++) {
            vm.deal(users[i], 20 ether);
            deal(weth, users[i], 10e18);
            deal(wsteth, users[i], 10e18);
        }
    }

    function _depositWithdrawKeroseneVault() internal {

        vm.startPrank(address(user2));

        address unboundedKerosineVault = address(contracts.unboundedKerosineVault);
        contracts.vaultManager.addKerosene(646, unboundedKerosineVault);    //<-- Call fails here

        //IERC20(weth).approve(address(contracts.vaultManager), 3e18);
        //contracts.vaultManager.deposit(646, unboundedKerosineVault, 3e18);

        //contracts.unboundedKerosineVault.id2asset(646);

        vm.stopPrank();
    }
}

TOOLS USED

Foundry

RECOMMNEDATION

The logic should point to the Licenser to perform checks instead of the Kerosene Manager. The script already licenses the unbounded kerosene vault using the Licenser but the addKerosene() function does not check in the Licenser.

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

Assessed type

DoS