DYAD - favelanky's results

Lines of code

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

Vulnerability details

Impact

Users will not be able to withdraw their funds until malicious user stops the DoS.

Proof of Concept

The current implementation of deposit and withdraw has an idToBlockOfLastDeposit mapping which doesn't allow to withdrawal of funds if they were deposited in the same block.

No need to be an owner of the dNFT to be able to deposit, see the isValidDNft modifier. Such implementation allows a malicious user to deposit a small number of tokens whenever the user wants to withdraw. Moreover, a malicious user can create a smart contract that will reflect the Vault interface, to not spend tokens. This is possible because the deposit function doesn't check the user-provided vault address.

    function deposit(uint256 id, address vault, uint256 amount) external isValidDNft(id) {
        idToBlockOfLastDeposit[id] = block.number;
        Vault _vault = Vault(vault);
        // @med if the token with fee => should track otherwise
        _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
        _vault.deposit(id, amount);
    }

    modifier isValidDNft(uint256 id) {
        if (dNft.ownerOf(id) == address(0)) revert InvalidDNft();
        _;
    }

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

Tools Used

Manual Analysis

Recommended Mitigation Stepss

Consider changing the modifier on the deposit function to isDnftOwner.

Assessed type

DoS

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#L250-L267

Vulnerability details

Impact

The user will not be able to withdraw a significant amount of Kerosine tokens at once.

Proof of Concept

The Kerosine token can be used to keep the debt overcollaterized. The main issue is depositing too many of these tokens will result in the unavailability to withdraw them later even if the debt is overcollaterized.

Let's see this in an example:

1. Alice deposited 100$ in Weth and 1000$ in Kerosine tokens (to have overcollaterized debt).
2. Let's imagine, that the price doesn't change much over time (doesn't even matter).
3. Later, Alice decides to withdraw the Kerosine tokens (tracing withdraw function):
   1. dyadMinted = 0 (she didn't mint any dyad tokens yet)
   2. value = 1000$ (Kerosine tokens)
   3. if (getNonKeroseneValue(id) - value < dyadMinted) will result in if (100 - 1000 < 0) which will revert the transaction.

If the price of the Kerosine tokens goes up or price of ETH goes down or Alice decides to mint some dyad tokens, the situation will be even worse. The solution is to withdraw small amounts of Kerosine tokens. The number of transactions to complete the withdrawal can be very high. This is not the intended behavior and can significantly affect the user experience.

    function withdraw(uint256 id, address vault, uint256 amount, address to) public isDNftOwner(id) {
        if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
        uint256 dyadMinted = dyad.mintedDyad(address(this), id);
        Vault _vault = Vault(vault);
        uint256 value =
            amount * _vault.assetPrice() * 1e18 / 10 ** _vault.oracle().decimals() / 10 ** _vault.asset().decimals();
        // @audit substraction non kerosine value from the kerosine value
        if (getNonKeroseneValue(id) - value < dyadMinted) revert NotEnoughExoCollat();
        _vault.withdraw(id, to, amount);
        if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow();
    }

Tools Used

Manual Analysis

Recommended Mitigation Steps

If the user wants to withdraw the Kerosine tokens, the if (getNonKeroseneValue(id) - value < dyadMinted) check should not be used. Only the collaterization ratio should be checked.

Assessed type

Math