Liquidation bonus logic is wrong

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

When a liquidator liquidates a user, he will pay his debt and must receive the debt + 20% bonus in form of collateral (from the user). But now the 20% bonus is based on the user’s (collateral - debt), which removes the entire incentive for liquidation.

Proof of Concept

From Docs:

liquidate() will first check if the user with the supplied id is for liquidation, then take the user's debt to cover from mintedDyad and burn it from the liquidator's balance. From there, the calculation of the liquidation bonus begins.

Let's look at this example (same as in the test below):

• UserA will have $135 collateral and $100 debt
• Liquidator assumes that he will receive the $100 debt + 20% of $100 ($20) = $120 as collateral

But it will actually calculate 20% of (UserA collateral - UserA debt), which in this case would be 20% of $35 = $7

• cappedCr will be 1.35e18
• liquidationEquityShare = (1.35e18 - 1e18) * 0.2e18 / 1e18 = 0.35e18 * 0.2e18 / 1e18 = 0.07e18
• liquidationAssetShare = *`(0.07e18 + 1e18) 1e18 / 1.35e18  ≈ 0.79e18 (107/135)`**

This 0.79e18 or more precisely 107/135 is how much of user’s collateral the liquidator will receive and that is $135 * (107/135) = $107.

As we can see for $100 repaid he will only get $7 collateral bonus collateral, which confirms our state that the 20% bonus is based on (UserA collateral - UserA debt).

function liquidate(
  uint id,
  uint to
) 
  external 
    isValidDNft(id)
    isValidDNft(to)
  {
    uint cr = collatRatio(id);
    if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();
    dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id));

    uint cappedCr               = cr < 1e18 ? 1e18 : cr;
    uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD);
    uint liquidationAssetShare  = (liquidationEquityShare + 1e18).divWadDown(cappedCr);

    uint numberOfVaults = vaults[id].length();
    for (uint i = 0; i < numberOfVaults; i++) {
        Vault vault      = Vault(vaults[id].at(i));
        uint  collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare);
        vault.move(id, to, collateral);
    }
    emit Liquidate(id, msg.sender, to);
}

Coded POC

The test will cover the same as case we explained above.

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_can_exit_even_if_cr_low -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_liquidation_bonus_is_wrong() public {
        address liquidator = makeAddr("Liquidator");
        address alice = makeAddr("Alice");

        wethOracle.setPrice(1e8); // Using 1$ for weth for better understanding
        uint256 liquidatorNft = mintDNFTAndDepositToWethVault(liquidator, 2e18, 1e18);
        uint256 aliceNft = mintDNFTAndDepositToWethVault(alice, 1.5e18, 1e18);

        wethOracle.setPrice(0.9e8);
        assertEq(vaultManagerV2.collatRatio(aliceNft), 1.35e18);

        console.log("Liquidator collateral before liquidate Alice:", vaultManagerV2.getNonKeroseneValue(liquidatorNft));
        vm.prank(liquidator);
        vaultManagerV2.liquidate(aliceNft, liquidatorNft);
        console.log("Liquidator collateral after liquidate Alice: ", vaultManagerV2.getNonKeroseneValue(liquidatorNft));
        // The liquidator receives only 106.9999999 collateral in return.
    }
}

Logs:
  Liquidator collateral before liquidate Alice: 1800000000000000000
  Liquidator collateral after liquidate Alice:  2869999999999999999

Tools Used

Manual Review

Recommended Mitigation Steps

The bonus should be based on the burned user debt and then must send the liquidator the percentage of liquidated user collateral equal to the burned debt + 20% bonus.

This is an example implementation, which gives the desired 20% bonus from the right amount, but need to be tested for further issues.

function liquidate(
  uint id,
  uint to
) 
  external 
    isValidDNft(id)
    isValidDNft(to)
  {
    uint cr = collatRatio(id);
+   uint userCollateral = getTotalUsdValue(id);
    if (cr >= MIN_COLLATERIZATION_RATIO) revert CrTooHigh();
    dyad.burn(id, msg.sender, dyad.mintedDyad(address(this), id));

-   uint cappedCr               = cr < 1e18 ? 1e18 : cr;

+   uint liquidationEquityShare = 0;
+   uint liquidationAssetShare = 1e18;
+   if (cr >= 1.2e18) {
+     liquidationEquityShare = (dyad.mintedDyad(address(this), id)).mulWadDown(LIQUIDATION_REWARD);
+     liquidationAssetShare  = (dyad.mintedDyad(address(this), id) + liquidationEquityShare).divWadDown(userCollateral);
+   }

-   uint liquidationEquityShare = (cappedCr - 1e18).mulWadDown(LIQUIDATION_REWARD);
-   uint liquidationAssetShare  = (liquidationEquityShare + 1e18).divWadDown(cappedCr);

    uint numberOfVaults = vaults[id].length();
    for (uint i = 0; i < numberOfVaults; i++) {
        Vault vault      = Vault(vaults[id].at(i));
        uint  collateral = vault.id2asset(id).mulWadUp(liquidationAssetShare);
        vault.move(id, to, collateral);
    }
    emit Liquidate(id, msg.sender, to);
}

Assessed type

Math

[c4-pre-sort]

JustDravee marked the issue as duplicate of #906

[c4-pre-sort]

JustDravee marked the issue as high quality report

[c4-judge]

koolexcrypto marked the issue as not a duplicate

[c4-judge]

koolexcrypto marked the issue as unsatisfactory: Invalid

[c4-judge]

koolexcrypto removed the grade

[c4-judge]

koolexcrypto marked the issue as duplicate of #75

[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