DYAD - ahmedaghadi's results

Lines of code

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

Vulnerability details

Impact

An attacker can call VaultManagerV2::deposit function with a fake vault ( or deposit 1 wei in a vault ) for a particular DNft token every block which will lead to user not being able to withdraw due DepositedInSameBlock revert in VaultManagerV2::withdraw function.

The VaultManagerV2::deposit function is as follows ( https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L119 ):

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

Here, there's no check for whether msg.sender is the owner of the DNft token or not. So, anyone can call VaultManagerV2::deposit function for a particular DNft token. Also, there's no check for whether the vault is a valid vault or not ( i.e vault is licensed or not ). So anyone can call this function with arbitrary amount as attacker's fake vault will have a dummy asset::transferFrom function which will always return true. And even if there's check for whether the vault is a valid vault or not, attacker can deposit 1 wei in the vault every block to restrict the availability of VaultManagerV2::withdraw function for a particular DNft token.

The VaultManagerV2::withdraw function is as follows ( https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L134 ):

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

Here, the if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock(); check will revert if user has deposited in the same block. So, attacker can restrict the availability of VaultManagerV2::withdraw function for a particular DNft token by depositing in the same block every block.

The cost of attack is only gas fees for calling VaultManagerV2::deposit function every block.

Proof of Concept

Consider the following scenario:

• (Optional step) Attacker create a dummy contract similar to a Vault contract. If this step is to be skipped then the cost of attack will be increased by 1 wei ( which kinda worth zero value ).
• Attacker calls VaultManagerV2::deposit function for a particular DNft token with a dummy vault ( or deposit 1 wei in a legit vault ) every block.
• Thus, user won't be able to call VaultManagerV2::withdraw function for that particular DNft token due to DepositedInSameBlock revert in VaultManagerV2::withdraw function.

Tools Used

Foundry and Manual Review

Recommended Mitigation Steps

It can be fixed by only allowing the owner of the DNft token to call VaultManagerV2::deposit function.

So the VaultManagerV2::deposit function can be updated as follows:

    function deposit(
    uint    id,
    address vault,
    uint    amount
    )
    external
        isValidDNft(id)
    {
+    require(dNft.ownerOf(id) == msg.sender, "VaultManagerV2: Only owner can deposit");
    idToBlockOfLastDeposit[id] = block.number;
    Vault _vault = Vault(vault);
    _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
    _vault.deposit(id, amount);
    }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The VaultManagerV2::withdraw function is as follows ( https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L134 ):

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

KerosineVault such as BoundedKerosineVault and UnboundedKerosineVault doesn't have any function named oracle thus the VaultManagerV2::withdraw function will revert if user tries to withdraw any amount of KEROSENE assets from the vault.

Proof of Concept

NOTE: There's another issue due to which BoundedKerosineVault and UnboundedKerosineVault can't be added to VaultManagerV2 contract ( I've made another submission for that in QA report titled "Can't add KerosineVault through VaultManagerV2::addKerosene function" ). So, the following changes are done in VaultManagerV2::addKerosene function to create a working POC:

    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 (!vaultLicenser.isLicensed(vault))                   revert VaultNotLicensed();
    if (!vaultsKerosene[id].add(vault))                     revert VaultAlreadyAdded();
    emit Added(id, vault);
    }

NOTE: This is just a temporary change to create a working POC. Actual fix is different and more complex ( thus out of scope for this current submission ) and is mentioned in the QA report ( as mentioned in previous NOTE ).

Add this to a newly created test file:

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

import "forge-std/Test.sol";
import "forge-std/console.sol";
import {DeployBase, Contracts} from "../script/deploy/DeployBase.s.sol";
import {Parameters} from "../src/params/Parameters.sol";
import {DNft} from "../src/core/DNft.sol";
import {Dyad} from "../src/core/Dyad.sol";
import {Licenser} from "../src/core/Licenser.sol";
import {VaultManager} from "../src/core/VaultManager.sol";
import {Vault} from "../src/core/Vault.sol";
import {Payments} from "../src/periphery/Payments.sol";
import {OracleMock} from "./OracleMock.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
import {IAggregatorV3} from "../src/interfaces/IAggregatorV3.sol";
import {ERC20} from "@solmate/src/tokens/ERC20.sol";

import {Kerosine} from "../src/staking/Kerosine.sol";
import {VaultManagerV2} from "../src/core/VaultManagerV2.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 {KerosineDenominator} from "../src/staking/KerosineDenominator.sol";

contract AuditTest is Test, Parameters {
    address owner = address(0x123AABB);
    address user = address(0x844);

    DNft dNft;
    Licenser vaultManagerLicenser;
    Licenser vaultLicenser;
    Dyad dyad;
    VaultManagerV2 vaultManager;
    Payments payments;

    // weth
    Vault wethVault;
    ERC20Mock weth;
    OracleMock wethOracle;

    // wsteth
    VaultWstEth wstEthVault;
    ERC20Mock wstEth;
    OracleMock wstEthOracle;

    Kerosine kerosine;
    KerosineManager kerosineManager;
    UnboundedKerosineVault unboundedKerosineVault;
    BoundedKerosineVault boundedKerosineVault;
    KerosineDenominator kerosineDenominator;

    function setUp() public {
        vm.startPrank(owner);
        dNft = new DNft();

        weth = new ERC20Mock("WETH-TEST", "WETHT");

        wethOracle = new OracleMock(1000e8);

        wstEth = new ERC20Mock("WSTETH-TEST", "WSTETHT");

        wstEthOracle = new OracleMock(1e6);

        vaultManagerLicenser = new Licenser();

        vaultLicenser = new Licenser();

        dyad = new Dyad(vaultManagerLicenser);

        kerosine = new Kerosine();

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

        wethVault = new Vault(
            vaultManager,
            weth,
            IAggregatorV3(address(wethOracle))
        );

        wstEthVault = new VaultWstEth(
            vaultManager,
            wstEth,
            IAggregatorV3(address(wstEthOracle))
        );

        kerosineManager = new KerosineManager();

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

        vaultManager.setKeroseneManager(kerosineManager);

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

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

        kerosineDenominator = new KerosineDenominator(kerosine);

        unboundedKerosineVault.setDenominator(kerosineDenominator);

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

        vaultManagerLicenser.add(address(vaultManager));
        vm.stopPrank();
    }

    function mint_dNft(address to) public returns (uint256) {
        return dNft.mintNft(to);
    }

    modifier mint_weth(address to, uint256 amount) {
        weth.mint(to, amount);
        _;
    }

    modifier mint_wsteth(address to, uint256 amount) {
        wstEth.mint(to, amount);
        _;
    }

    modifier mint_kerosine(address to, uint256 amount) {
        vm.prank(owner);
        kerosine.transfer(to, amount);
        _;
    }

    // This test will fail as withdraw function in VaultManagerV2 expect kerosineVault to have function named `oracle` whereas UnboundedKerosineVault doesn't have any function `oracle` ( or any public variable named `oracle` )
    function test_withdraw_kerosine()
        public
        mint_weth(user, 10e18)
        mint_kerosine(user, 10e18)
    {
        uint tokenId = mint_dNft(user);
        vm.prank(user);
        vaultManager.add(tokenId, address(wethVault));
        vm.prank(user);
        vaultManager.addKerosene(tokenId, address(unboundedKerosineVault));

        vm.prank(user);
        kerosine.approve(address(vaultManager), 10e18);

        vm.prank(user);
        weth.approve(address(vaultManager), 10e18);

        vm.prank(user);
        vaultManager.deposit(tokenId, address(unboundedKerosineVault), 10e18);

        assertEq(kerosine.balanceOf(address(unboundedKerosineVault)), 10e18);

        vm.roll(block.number + 1);

        vm.prank(user);
        vaultManager.deposit(tokenId, address(wethVault), 10e18);

        assertEq(weth.balanceOf(address(wethVault)), 10e18);

        vm.roll(block.number + 1);

        vm.prank(user);
        vaultManager.withdraw(
            tokenId,
            address(unboundedKerosineVault),
            10e18,
            user
        );

        assertEq(kerosine.balanceOf(address(unboundedKerosineVault)), 0);
        assertEq(kerosine.balanceOf(user), 10e18);
    }

    receive() external payable {}

    function onERC721Received(
        address,
        address,
        uint256,
        bytes calldata
    ) external pure returns (bytes4) {
        return 0x150b7a02;
    }
}

You can run the test by:

forge test --mt test_withdraw_kerosine -vvvv

And it will fail with the following error:

    │   ├─ [249] Kerosine::decimals() [staticcall]
    │   │   └─ ← 18
    │   ├─ [214] UnboundedKerosineVault::oracle() [staticcall]
    │   │   └─ ← EvmError: Revert
    │   └─ ← EvmError: Revert
    └─ ← EvmError: Revert

Tools Used

Foundry and Manual Review

Recommended Mitigation Steps

It can be fixed by deploying an Oracle contract for KerosineVault and that Oracle contract should have a function named decimals which will returns 8 due to the fact that in UnboundedKerosineVault::assetPrice function, 1e8 is multiplied with numerator.

Assessed type

Error