DYAD - bbl4de's results

Lines of code

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

Vulnerability details

Impact

In VaultManagerV2::withdraw() function, to calculate the value of assets to withdraw _vault.oracle().decimals() is called:

uint value = (amount * _vault.assetPrice() * 1e18) /
@>          10 ** _vault.oracle().decimals() /
            10 ** _vault.asset().decimals();

However, the issue is that neither bounded or unbounded kerosene vaults have an oracle. Considering that Vault.kerosine.unbounded::withdraw is protected by the onlyVaultManager modifier, it becomes impossible to withdraw your unbounded kerosene as any call will revert.

Proof of Concept

To showcase this evm revert create a ./test/BaseTestV2.t.sol test file and use the code from this gist where the new VaultManagerV2 was deployed together with Mock kerosene and vaults. To run the test:

forge test --mt test_withdrawFails -vvv

Tools Used

Manual review

Recommended Mitigation Steps

One possible solution is to add another withdraw function in VaultManagerV2 assigned strictly to kerosene vaults, that would automatically input the required decimals. Careful reconsideration of the value calculation method is necessary.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

VaultManagerV2::addKerosene() requires the vault to be licensed in the keroseneManager:

        if (!keroseneManager.isLicensed(vault)) revert VaultNotLicensed();

This however, breaks adding kerosene vaults functionality completely. Licensed vaults in kerosineManager are simply ones that are in the KerosineManager::vaults address set, which is used to calculate kerosene price based on EXOGENOUS collateral.

Specifically every call to VaultManagerV2::getKeroseneValue will fail, because of the following line:

if (keroseneManager.isLicensed(address(vault))) {
	usdValue = vault.getUsdValue(id);
}

where KerosineVault::getUsdValue() calls assetPrice(), which finally attempts to call vault.oracle() function which is impossible for kerosene vaults. This vulnerability does not cause incorrect calculations, but rather blocks entirely any function in VaultManagerV2 that internally calls collatRatio() : mintDyad() and liquidate() due to an evm error revert.

In short, either kerosene vaults would be impossible to add to your Note NFT - because of the incorrect licensing check in addKerosene(), or if they were somehow added - any user could block liquidation functionality at will, protecting themselves from the price action of other collateral assets.

Proof of Concept

To showcase this evm revert create a ./test/BaseTestV2.t.sol test file and use the code from this gist where the new VaultManagerV2 was deployed together with mock kerosene and vaults. To run the test:

forge test --mt test_keroseneVaults

Detailed thought process is included in the comments.

Tools Used

Manual review

Recommended Mitigation

To mitigate this issue. calling addKerosene() should not be limited by whether the vault is licensed in the kerosene manager. However, to keep the licensing for kerosene vaults as well, consider the following change in addKerosene():

 function addKerosene(uint id, address vault) external isDNftOwner(id) {
        if (vaultsKerosene[id].length() >= MAX_VAULTS_KEROSENE)
            revert TooManyVaults();
-       if (!keroseneManager.isLicensed(vault)) revert VaultNotLicensed();
+       if (!vaultManager.isLicensed(vault)) revert VaultNotLicensed();
        if (!vaultsKerosene[id].add(vault)) revert VaultAlreadyAdded();
        emit Added(id, vault);
    }

and simply use licensing from the Licenser contract to ensure vaults that are available for the users are secure.

Assessed type

Other