DYAD - 0x77's results

Lines of code

https://github.com/DyadStablecoin/contracts/blob/1dda63c007f8e59b3726a271b26aafa6ea8993e0/src/core/VaultManagerV2.sol#L119

Vulnerability details

Description

A critical vulnerability has been identified in the withdraw function of the VaultManagerV2 smart contract, where a key state check idToBlockOfLastDeposit[id] == block.number determines if withdrawals can proceed. This flaw allows attackers to manipulate the idToBlockOfLastDeposit state by sending a VaultManagerV2::deposit transaction before the user's withdrawal request. The lack of an isDNftOwner check in the deposit function enables non-owners to execute this function, thus allowing attackers to modify critical states and block legitimate withdrawals at a low cost.

Impact Assessment

This vulnerability enables attackers to exploit the front-running technique to alter the contract state of VaultManagerV2::idToBlockOfLastDeposit, causing legitimate withdrawal attempts to fail. This not only threatens the security of user funds but also severely compromises the contract's usability and trustworthiness.

Proof of Concept

1. At block.number == 1, user Alice calls deposit to deposit funds, setting idToBlockOfLastDeposit[0] == 1.
2. When block.number == 10, Alice attempts to withdraw her funds.
3. Attacker Bob, monitoring Alice's withdrawal operations, initiates a deposit using the same tokenID before Alice's withdrawal attempt. According to the ERC20 standard, transactions can be executed even if the amount sent is zero. Since the deposit function lacks proper asset ownership verification, Bob can successfully submit the transaction without actually holding any assets, thereby updating idToBlockOfLastDeposit[0] to the current block number, 10.
4. As miners process Alice's withdrawal request, the condition idToBlockOfLastDeposit[id] == block.number fails, resulting in a failed withdrawal for Alice.

Proof of code

By adding the following code to the vaultManagerV2.t.sol test file, we simulate the attack described above:

contract VaultManagerV2Test is Test, BaseTest {

    address alice;
    address bob;

    function testDeposited() public {
        vm.roll(1);
        alice = vm.addr(1);
        bob = vm.addr(2);
        vm.startPrank(alice);
        dNft.mt();
        vaultManagerV2.deposit(0, address(daiVault), 0);
        assert(vaultManagerV2.idToBlockOfLastDeposit(0) == 1);
        vm.stopPrank();

        vm.roll(10);
        vm.startPrank(bob);
        vaultManagerV2.deposit(0, address(daiVault), 0);
        assert(vaultManagerV2.idToBlockOfLastDeposit(0) == 10);
        vm.stopPrank();

        vm.startPrank(alice);
        vaultManagerV2.withdraw(0, address(daiVault), 0, a);
        vm.stopPrank();
    }

}

Tools Used

foundry

Recommended mitigation

To prevent future front-running attacks, it is advised to add an isDNftOwner check to the deposit function, ensuring that only asset owners can invoke this function. The amendment is as follows:

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

Assessed type

DoS