c4-bot-4 commented 6 months ago

Lines of code

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

Vulnerability details

Impact

Denial of Service for the Victim User of withdraw & redeemDyad functions of VaultManagerV2.sol .

Proof of Concept

The current implementation of VaultManagerV2.sol#withdraw function has the following check before withdrawal from Vault where the state idToBlockOfLastDeposit is being checked as shown below :

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

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

This check is implemented to prevent flashloan attacks . But the problem arises when a Malicious user can simply deposit a small amount of token to the Vault to prevent the withdrawal process . This arises due to the fact that the VaultManagerV2.sol#deposit does not have a strict access modifier to prevent such attacks . Let's see how it can be executed . As shown below the VaultManagerV2.sol#deposit function has a lineant function access modifier which just checks whether the dnft used exists or not . And the function has this line of code where it changes the state idToBlockOfLastDeposit to current block.number as shown below.

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

 function deposit(
    uint    id,
    address vault,
    uint    amount
  ) 
    external 
      isValidDNft(id)                                     <@audit
  {
    idToBlockOfLastDeposit[id] = block.number;            <@audit
    Vault _vault = Vault(vault);
    _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
    _vault.deposit(id, amount);
  }

The isValidDNft is shown below : https://github.com/code-423n4/2024-04-dyad/blob/main/src/core/VaultManagerV2.sol#L42C1-L44C4

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

So whenever a Victim User tries to withdraw the asset's from the Vault , a Malicious User can frontrun the transaction and deposit 1 wei of token to the Vault to change the idToBlockOfLastDeposit to current block.number , thereby preventing the Victim User from withdrawing the funds .

Tools Used

Manual Review

Recommended Mitigation Steps

To prevent this attack the access modifier for the deposit function should be made more strict . Either the access modifier should be changed from isValidDNft to isDNftOwner or the User should be allowed to create a allowlist of Trusted Users , who would be allowed to deposit on behalf of the DNFTOwner .

Assessed type

Access Control

c4-pre-sort commented 6 months ago

JustDravee marked the issue as duplicate of #489

c4-pre-sort commented 6 months ago

JustDravee marked the issue as sufficient quality report