DYAD - blutorque's results

Lines of code

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

Vulnerability details

Impact

The deposit() function allows anyone to deposit on behalf of other user(holding dNFT). This result in updating the idToBlockOfLastDeposit[id] to current block.number. As a result, any future withdrawal for that id in the same block will revert.

Actually, the idToBlockOfLastDeposit[id] is used to prevent from flashloan based attack, which require deposit and withdraw to be in the same txn. However, this check can also maliciously be used against legitmate user who want to withdraw their assets.

Proof of Concept

1. A user sent a withdraw() txn to VaultManagerV2,
2. Attacker saw the txn in mempool and sent a deposit() txn for that user id, with 1 wei of assets as amount,
3. Since, the withdraw call got frontrunned by the deposit txn, the idToBlockOfLastDeposit[id] got updated,
4. The withdraw txn got reverts, due to err DepositedInSameBlock().

Tools Used

Manual review

Recommended Mitigation Steps

Allow idToBlockOfLastDeposit[id] to update, only when the caller is the ownerOf(id). This way, anyone can make deposit to id without any interruption.

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

Assessed type

DoS