Vault Manager doesn't have any slippage protection when redeeming/withdrawing assets

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

When withdrawing/redeeming whether exogenous collateral or Kerosene, the amount which the msg.sender will receive or the so-called value is calculated based on the current asset price by fixating the DYAD to the value of a dollar. There's no mechanism in which the user can control how much assets they receive in return when redeeming their DYAD or when withdrawing the assets.

Proof of Concept

This can be prone to users being damaged in multiple ways, including:

• Kerosene price is dependent on the amount of exogenous collateral in vaults, as well as any movements from the multisig, which can significantly affect its price, during large movements, if a user's transaction was frontrun/behind a very large deposit/withdrawal, etc.

• Besides a minimum amount out, the protocol doesn't utilize any deadlines as well which can be also affected by oracle price movements and/or transactions that have been sitting for too long in the mempool.

When redeeming DYAD for assets, this is how the collateral's value is calculated:

  function redeemDyad(uint256 id, address vault, uint256 amount, address to)
        external
        isDNftOwner(id)
        returns (uint256)
    {
        dyad.burn(id, msg.sender, amount);
        Vault _vault = Vault(vault);
        uint256 asset =
            amount * (10 ** (_vault.oracle().decimals() + _vault.asset().decimals())) / _vault.assetPrice() / 1e18;
        withdraw(id, vault, asset, to);
        emit RedeemDyad(id, vault, amount, to);
        return asset;

We can see that the user has no control over how much asset they receive in exchange for their DYAD. DYAD is fixated to be worth a dollar, and then calculations are made how much assets will the user receive in exchange for the amount which is in DYAD/USD.

This can be damaging to users as they have no control over the minimum amount out, since large transactions can affect Kerosene prices and/or steep movements in the oracle prices fetched.

From the formula in which the Kerosene asset price is calculated, we can see that it's heavily dependent on the current balance of the vaults:

 for (uint256 i = 0; i < numberOfVaults; i++) {
            Vault vault = Vault(vaults[i]);
            tvl += vault.asset().balanceOf(address(vault)) * vault.assetPrice() * 1e18
                / (10 ** vault.asset().decimals()) / (10 ** vault.oracle().decimals());
        }

Meaning that users will have no control of how much Kerosene they get in exchange for their DYAD if they've been frontrunned by a large withdrawal as one of the examples.

Tools Used

Manual Review

Recommended Mitigation Steps

Include a minimum amount out parameter. An additional layer of protection would be a deadline parameter as well in order to prevent executing stale transactions which have been in the mempool for a long time.

Assessed type

Other

[c4-pre-sort]

JustDravee marked the issue as duplicate of #255

[c4-pre-sort]

JustDravee marked the issue as sufficient quality report

[c4-judge]

koolexcrypto marked the issue as unsatisfactory: Invalid