DYAD - ducanh2706's results

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L133-L153

Vulnerability details

[M-2] User can not withdraw their deposited kerosene due to VaultManager::withdraw() function is designed only for deposited exogenous collateral

Description

User can not withdraw their deposited kerosene due to VaultManager::withdraw() function is designed only for deposited exogenous collateral, as can be seen here:

function withdraw(
    uint    id,
    address vault,
    uint    amount,
    address to
  ) 
    public
      isDNftOwner(id)
  {
    ...
    uint value = amount * _vault.assetPrice() 
                  * 1e18 
                  / 10**_vault.oracle().decimals() 
                  / 10**_vault.asset().decimals();
    ...
  }

As the kerosene-vault doesn't have oracle() field, this function would be reverted.

Impact

This would lead to user's deposited kerosene can never be withdrawed.

Proof of Concept

Here is the local test I used.

    function setUp() public {
        helper = new DeployLocal().run();
        aggregator = helper.aggregator;
        kerosene = helper.kerosene;
        nft = helper.nft;
        dyad = helper.dyad;
        licenser = helper.licenser;
        vaultKeroseneLicenser = helper.vaultKeroseneLicenser;
        vaultManager = helper.vaultManager;
        kerosineManager = helper.kerosineManager;
        unboundedKerosineVault = helper.unboundedKerosineVault;
        kerosineDenominator = helper.kerosineDenominator;
        ethVault = helper.ethVault;
        weth = helper.weth;

        kerosene.mint(MAINNET_OWNER, 1000000000 ether);

        vm.prank(randomAddress);
        uint256 id = mintNFT(randomAddress);


        vm.startPrank(MAINNET_OWNER);
        kerosene.transfer(randomAddress, 5e7 ether);
        vm.stopPrank();

        vm.prank(address(vaultManager));
        dyad.mint(id, randomAddress, 5e6 ether);
    }

    function mintNFT(address player) public returns (uint id){  
        vm.deal(player, 1000000 ether);
        // mintNFT
        id = nft.mintNft(player);
    }

    function testCantWithdrawKerosene() public {
        vm.prank(randomAddress);
        kerosene.transfer(user, 5e6 ether);

        vm.startPrank(user);
        uint id = mintNFT(user);
        kerosene.approve(address(vaultManager),5e6);
        vaultManager.deposit(id, address(unboundedKerosineVault), 5e6);
        vaultManager.addKerosene(id, address(unboundedKerosineVault));
        vm.stopPrank();

        vm.roll(block.number + 2);

        vm.startPrank(user);
        vaultManager.withdraw(id, address(unboundedKerosineVault), 5e6, user);
        vm.stopPrank();

    }

The log is:

    ├─ [9251] VaultManagerV2::withdraw(1, UnboundedKerosineVault: [0x978e3286EB805934215a88694d80b09aDed68D90], 5000000 [5e6], user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D]) 
    │   ├─ [557] DNft::ownerOf(1) [staticcall]
    │   │   └─ ← user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D]
    │   ├─ [2623] Dyad::mintedDyad(VaultManagerV2: [0xD718d5A27a29FF1cD22403426084bA0d479869a0], 1) [staticcall]
    │   │   └─ ← 0
    │   ├─ [261] UnboundedKerosineVault::asset() [staticcall]
    │   │   └─ ← ERC20Mock: [0xA8452Ec99ce0C64f20701dB7dD3abDb607c00496]
    │   ├─ [271] ERC20Mock::decimals() [staticcall]
    │   │   └─ ← 18
    │   ├─ [214] UnboundedKerosineVault::oracle() [staticcall]
    │   │   └─ ← "EvmError: Revert"
    │   └─ ← "EvmError: Revert"
    └─ ← "EvmError: Revert"

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 3.69ms
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Tool used

Manual Review

Recommended Mitigation Steps

Just implement another VaultManager::withdrawKerosene() to handle the withdraw of kerosene.

+   function withdrawKerosene(uint id, address vault, uint amount, address to) public isDNftOwner(id) {
+       if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
+       Vault _vault = Vault(vault);
+       _vault.withdraw(id, to, amount);
+       if (collatRatio(id) < MIN_COLLATERIZATION_RATIO)  revert CrTooLow(); 
+   }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L222-L227

Vulnerability details

[H-1] There Is a Scenario Where There's No Incentive to Liquidate Undercollateralized Assets When Kerosene Is Not Awarded to the Liquidator

Description

There's a scenario where the liquidator receives less than they are required to pay due to most of the collateral of the undercollateralized asset being kerosene.

Impact

This could result in plummeting prices of exogenous assets never being liquidated, posing a significant risk to the protocol. Such assets may remain undercollateralized indefinitely, potentially compromising the protocol's integrity.

Proof of Concept

Below is the test I wrote, assuming the price of WETH drops:

contract LocalTest is Test, Parameters {
    // ... [snip] ... (Your setup code remains unchanged)

    function testNoIncentive() public {
        Deposit(user);
        Deposit(liquidator);

        // Let 1000 players deposit into the vault
        for (uint i = 3; i <= 1000; i++) {
            Deposit(address(uint160(i)));
        }

        // The price of WETH drops.
        aggregator.updateAnswer(1850e8);
        ///////////////////////////

        console.log("Collateral Ratio of 0: ", vaultManager.collatRatio(0));

        console.log("Minted Dyad before (0): ", dyad.mintedDyad(address(vaultManager), 0) / 1e18);
        console.log("Minted Dyad before (1): ", dyad.mintedDyad(address(vaultManager), 1) / 1e18);
        console.log("Total kerosene value before (0): ", vaultManager.getKeroseneValue(0) / 1e18);
        console.log("Total kerosene value before (1): ", vaultManager.getKeroseneValue(1) / 1e18);
        console.log("Total non-kerosene value before (0): ", vaultManager.getNonKeroseneValue(0) / 1e18);
        console.log("Total non-kerosene value before (1): ", vaultManager.getNonKeroseneValue(1) / 1e18);
        console.log("Total asset before (0): ", vaultManager.getTotalUsdValue(0) / 1e18);
        console.log("Total asset before (1): ", vaultManager.getTotalUsdValue(1) / 1e18);

        vm.startPrank(liquidator);

        vaultManager.liquidate(0, 1);
        
        vm.stopPrank();

        console.log();

        console.log("Minted Dyad after (0): ", dyad.mintedDyad(address(vaultManager), 0) / 1e18);
        console.log("Minted Dyad after (1): ", dyad.mintedDyad(address(vaultManager), 1) / 1e18);
        console.log("Total kerosene value after (0): ", vaultManager.getKeroseneValue(0) / 1e18);
        console.log("Total kerosene value after (1): ", vaultManager.getKeroseneValue(1) / 1e18);
        console.log("Total non-kerosene value after (0): ", vaultManager.getNonKeroseneValue(0) / 1e18);
        console.log("Total non-kerosene value after (1): ", vaultManager.getNonKeroseneValue(1) / 1e18);
        console.log("Total asset after (0): ", vaultManager.getTotalUsdValue(0) / 1e18);
        console.log("Total asset after (1): ", vaultManager.getTotalUsdValue(1) / 1e18);        
    }
}

The logs are as follows:

Collateral Ratio of 0:  1466666666666666666
Minted Dyad before (0):  150000
Minted Dyad before (1):  150000
Total kerosene value before (0):  35000
Total kerosene value before (0):  35000
Total non-kerosene value before (0):  185000
Total non-kerosene value before (1):  185000
Total asset before (0):  220000
Total asset before (1):  220000

Minted Dyad after (0):  0
Minted Dyad after (1):  150000
Total kerosene value after (0):  35150
Total kerosene value after (1):  35150
Total non-kerosene value after (0):  47090
Total non-kerosene value after (1):  322909
Total asset after (0):  82240
Total asset after (1):  358059

Assuming the price of WETH initially is 3000, then drops to 1850, it can be seen that liquidator id(1) liquidates the asset of id(0) with 150000 of DYAD burned but only receives 358059 - 220000 = 138059.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this issue, we can consider allowing kerosene as a reward to the liquidator. There's no harm in this as the awarded kerosene does not impact its price.

function liquidate(
    uint id,
    uint to
) 
  external 
    isValidDNft(id)
    isValidDNft(to)
{
...
+     numberOfVaults = vaultsKerosene[id].length();
+
+     for (uint i = 0; i < numberOfVaults; i++){
+         Vault vault      = Vault(vaultsKerosene[id].at(i));
+         uint  collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare);
+         vault.move(id, to, collateral);
+     }
}

Assessed type

Other

Lines of code

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

Vulnerability details

[M-1] VaultManagerV2::add and VaultManagerV2::addKerosene Do Not Function Correctly, Resulting in Incorrect Calculation of VaultManagerV2::collatRatio

Description

The VaultManagerV2::add function is intended to add only wethVault and wstethVault. However, since the vaultLicenser also allows for unboundedKeroseneVault, the function cannot operate as designed. Additionally, the VaultManagerV2::addKerosene function checks for a valid vault using keroseneManager. However, the purpose of keroseneManager is to manage the exogenous vaults that affect the ERC20::Kerosene price.

Impact

This issue can lead to an incorrect calculation of VaultManagerV2::collatRatio. Vaults such as wethVault can be added to both the vaults and vaultsKerosene mappings, resulting in collateral being counted twice. This incorrect collateral calculation can lead to undercollateralized assets needing to be liquidated but unable to be.

Proof of Concept

Below is the test I added to ./test/fork/v2.t.sol, where player is a randomly generated address:

...
+   address public player = makeAddr("player");
...
+   function testcanAddWETHVaultToBothVaultsManager() public {
+       vm.deal(player, 1000 ether);
+       DNft nft = DNft(MAINNET_DNFT);
+       vm.prank(player);
+       uint id = nft.mintNft{value: 100 ether}(player);

+       vm.startPrank(player);

+       WETH(payable(MAINNET_WETH)).deposit{value: 1 ether}();
+       WETH(payable(MAINNET_WETH)).approve(address(contracts.vaultManager), 1 ether);
+       contracts.vaultManager.deposit(id, address(contracts.ethVault), 1 ether);

+       vaultManager.add(id, address(contracts.ethVault));
+       vaultManager.addKerosene(id, address(contracts.ethVault));

+       console.log("Total value of nonKeroseneVault is: ", vaultManager.getNonKeroseneValue(id));
+       console.log("Total value of keroseneVault is: ", vaultManager.getKeroseneValue(id));
+       console.log("Total asset of NFT is: ", vaultManager.getTotalUsdValue(id));

+       vm.stopPrank();
+   }

The logs are as follows:

Total value of nonKeroseneVault is: 3229060729920000000000
Total value of keroseneVault is: 3229060729920000000000
Total asset of NFT is: 6458121459840000000000

As observed, the collateral is counted twice.

Tools Used

Manual Review

Recommended Mitigation Steps

To address this issue, we should implement two separate licensers: one for non-Kerosene-vault and another for Kerosene-vault.

## Assessed type

Other