sherlock-admin commented 1 year ago

0xDjango

high

Pools that borrow same token as collateral can't be liquidated

Summary

Because of the require statements in the D3VaultLiquidation.liquidate() function, a pool that borrows the same token as its collateral can never be liquidated.

require(isPositiveNetWorthAsset(pool, collateral), Errors.INVALID_COLLATERAL_TOKEN);
require(!isPositiveNetWorthAsset(pool, debt), Errors.INVALID_DEBT_TOKEN);

If the pool has both collateral in TokenA and debt in TokenA, it's impossible for both require statements to pass.

Vulnerability Detail

Looking into the test file D3Funding.t.sol gives some clarity into the possibility of this vulnerability:

function testCanBeLiquidated() public {
    vm.startPrank(poolCreator);
    d3Proxy.makerDeposit(address(d3MM), address(token1), 41 * 1e8);
    d3MM.borrow(address(token1), 100 * 1e8);

    // pass
    vm.warp(315300000000);
    uint256 ratio = d3Vault.getCollateralRatio(address(d3MM));
    assertEq(ratio, 0);
    bool can = d3MM.checkCanBeLiquidated();
    assertEq(can, true);
}

The poolCreator deposits 41 token1
Then borrows 100 token1
Time passes
The checkCanBeLiquidated() call returns true

Looking at the checkCanBeLiquidated() function, it simply checks the collateral ratio is below the maintenance margin:

function checkCanBeLiquidated(address pool) public view returns (bool) {
    return getCollateralRatio(pool) < 1e18 + MM;
}

But when someone calls D3VaultLiquidation.liquidate(), it will always revert because the single TokenA can not satisfy both conditions isPositiveNetWorthAsset and !isPositiveNetWorthAsset.

function liquidate(
    address pool,
    address collateral,
    uint256 collateralAmount,
    address debt,
    uint256 debtToCover
) external nonReentrant {
    accrueInterests();

    require(!ID3MM(pool).isInLiquidation(), Errors.ALREADY_IN_LIQUIDATION);
    require(!checkBadDebtAfterAccrue(pool), Errors.HAS_BAD_DEBT);
    require(checkCanBeLiquidatedAfterAccrue(pool), Errors.CANNOT_BE_LIQUIDATED);
    require(isPositiveNetWorthAsset(pool, collateral), Errors.INVALID_COLLATERAL_TOKEN);
    require(!isPositiveNetWorthAsset(pool, debt), Errors.INVALID_DEBT_TOKEN);
    require(getPositiveNetWorthAsset(pool, collateral) >= collateralAmount, Errors.COLLATERAL_AMOUNT_EXCEED);

   ...

NOTE: It's also worth noting that the test file D3VaultLiquidation.t.sol contains tests for multiple different liquidation scenarios but does not have a single test case for the scenario where the pool only has a single token as both collateral and debt.

Impact

PoolCreators can borrow unlimited amounts without the ability to be liquidated

Code Snippet

https://github.com/sherlock-audit/2023-06-dodo/blob/main/new-dodo-v3/contracts/DODOV3MM/D3Vault/D3VaultLiquidation.sol#L42-L43

Tool used

Manual Review

Recommendation

This is tricky to mitigate due to the various functions that allow borrowing and swapping between different tokens. Perhaps the best action is to create a new function that can handle liquidating pools that only contain a single token as opposed to altering the current liquidate() function to support said scenario.

Attens1423 commented 1 year ago

A token can be used either as debt or collateral. Liquidators will not engage in unprofitable activities. In the case of a single token pool, if a liquidator sends 100 A tokens and receives only 95 A tokens in return, it is not cost-effective for the liquidation robot. Therefore, this situation will not be triggered.

Evert0x commented 1 year ago

The poolCreator deposits 41 token1 Then borrows 100 token1

Seems like this scenario is not realistic, how can someone borrow 100 tokens against 41 tokens?

0xffff11 commented 1 year ago

I agree with Evert's comments, this seems not possible, and according to sponsors comments, this should not be met neither. Will close as invalid for the preliminary results

fatherGoose1 commented 1 year ago

Escalate

This is a valid issue that seems to be misunderstood by all parties.

@Attens1423, this report states that if a pool creator borrows the same token as what they've provided as collateral, it cannot be liquidated. This report does not discuss liquidator profitability. It only highlights that a pool can not be liquidated under this specific circumstance.

@Evert0x @0xffff11, this borrow ratio is realistic and possible. Dodo provides leveraged borrowing pools. The code I referenced is directly from Dodo's own tests. https://github.com/sherlock-audit/2023-06-dodo/blob/a8d30e611acc9762029f8756d6a5b81825faf348/new-dodo-v3/test/DODOV3MM/D3MM/D3Funding.t.sol#L103-L114

    function testCanBeLiquidated() public {
        vm.startPrank(poolCreator);
        d3Proxy.makerDeposit(address(d3MM), address(token1), 41 * 1e8);
        d3MM.borrow(address(token1), 100 * 1e8);

        // pass
        vm.warp(315300000000);
        uint256 ratio = d3Vault.getCollateralRatio(address(d3MM));
        assertEq(ratio, 0);
        bool can = d3MM.checkCanBeLiquidated();
        assertEq(can, true);
    }

As explained in the report, if a pool creator:

Deposits Token1 as collateral
Borrows Token1 as debt
The pool CAN NOT BE LIQUIDATED. Even after debt accrues over time, the liquidate() function will always fail.

It can not be liquidated because the D3VaultLiquidation.liquidate() function contains checks that the collateral token needs to have positive value and debt token needs to have negative value in the pool:

https://github.com/sherlock-audit/2023-06-dodo/blob/a8d30e611acc9762029f8756d6a5b81825faf348/new-dodo-v3/contracts/DODOV3MM/D3Vault/D3VaultLiquidation.sol#L42-L43

        require(isPositiveNetWorthAsset(pool, collateral), Errors.INVALID_COLLATERAL_TOKEN);
        require(!isPositiveNetWorthAsset(pool, debt), Errors.INVALID_DEBT_TOKEN);

These statements can not pass if the collateral token is the same as the debt token.

Extra info: Upon testing, I have found even another blocker to this liquidation scenario. Because of the way collateral ratios are calculated, the pool actually goes from a state of not liquidateable straight to bad debt. This causes an even previous liquidation check to fail:

https://github.com/sherlock-audit/2023-06-dodo/blob/a8d30e611acc9762029f8756d6a5b81825faf348/new-dodo-v3/contracts/DODOV3MM/D3Vault/D3VaultLiquidation.sol#L40

        require(!checkBadDebtAfterAccrue(pool), Errors.HAS_BAD_DEBT);

Conclusion and POC After much testing, I have verified that single-token pools cannot be liquidated. However, it turned out that the previous check regarding bad debt caused the revert. This is because the collateral ratio goes from a massively positive value directly to 0 when the pool becomes liquidateable. I played with the following vm.warp() value in the POC for a long time. For a warp value smaller than the one below, the d3MM.checkCanBeLiquidated(); returns false. Once the warp value hits around the value provided, the result is true. However, actually attempting the liquidate() call returns D3VAULT_HAS_BAD_DEBT.

The following is the POC. It's the exact same as the Dodo-provided D3Funding.t.sol.testCanBeLiquidated(), but I have changed the warp time to be closer to the time at which the liquidation flips from false to true, AND I have added the actual call to liquidate() to demonstrate the failure.

IF you remove the require(!checkBadDebtAfterAccrue(pool), Errors.HAS_BAD_DEBT); check, then my original comment passes as intended. The test will return the Errors.INVALID_COLLATERAL_TOKEN) error.

/*

    Copyright 2023 DODO ZOO.
    SPDX-License-Identifier: Apache-2.0

*/

pragma solidity 0.8.16;

import "forge-std/Test.sol";
import "../../TestContext.t.sol";

//import {Errors} from "contracts/DODOV3MM/lib/Errors.sol";

contract D3FundingTest is TestContext {
    MockERC20 public tokenEx;
    MockChainlinkPriceFeed public tokenExChainLinkOracle;

    function setUp() public {
        contextBasic();
        setVaultAsset();
        mintPoolCreator();

        tokenEx = new MockERC20("TokenEx", "TKEx", 18);
        tokenExChainLinkOracle = new MockChainlinkPriceFeed("TokenEx/USD", 18);
        tokenExChainLinkOracle.feedData(24 * 1e18);
    }

    function testCanBeLiquidated() public {
        vm.startPrank(poolCreator);
        d3Proxy.makerDeposit(address(d3MM), address(token1), 41 * 1e8);
        d3MM.borrow(address(token1), 100 * 1e8);
        vm.stopPrank();
        logCollateralRatio(address(d3MM));

        // pass
        vm.warp(28153000);
        logCollateralRatio(address(d3MM));
        uint256 ratio = d3Vault.getCollateralRatio(address(d3MM));
        assertEq(ratio, 0);
        bool can = d3MM.checkCanBeLiquidated();
        assertEq(can, true);

        // Liquidation
        token1.mint(user2, 100 ether);
        vm.prank(user2);
        token1.approve(address(d3Vault), 100 ether);

        vm.prank(address(d3MM));
        token1.approve(address(d3Vault), type(uint256).max);

        vm.prank(user2);
        vm.expectRevert(bytes(Errors.D3VAULT_HAS_BAD_DEBT));
        d3Vault.liquidate(
            address(d3MM),
            address(token1),
            1 ether,
            address(token1),
            1 ether
        );
    }
}

sherlock-admin2 commented 1 year ago

Escalate

This is a valid issue that seems to be misunderstood by all parties.

@Attens1423, this report states that if a pool creator borrows the same token as what they've provided as collateral, it cannot be liquidated. This report does not discuss liquidator profitability. It only highlights that a pool can not be liquidated under this specific circumstance.

@Evert0x @0xffff11, this borrow ratio is realistic and possible. Dodo provides leveraged borrowing pools. The code I referenced is directly from Dodo's own tests. https://github.com/sherlock-audit/2023-06-dodo/blob/a8d30e611acc9762029f8756d6a5b81825faf348/new-dodo-v3/test/DODOV3MM/D3MM/D3Funding.t.sol#L103-L114
    function testCanBeLiquidated() public {
        vm.startPrank(poolCreator);
        d3Proxy.makerDeposit(address(d3MM), address(token1), 41 * 1e8);
        d3MM.borrow(address(token1), 100 * 1e8);

        // pass
        vm.warp(315300000000);
        uint256 ratio = d3Vault.getCollateralRatio(address(d3MM));
        assertEq(ratio, 0);
        bool can = d3MM.checkCanBeLiquidated();
        assertEq(can, true);
    }
As explained in the report, if a pool creator:

Deposits Token1 as collateral

Borrows Token1 as debt

The pool CAN NOT BE LIQUIDATED. Even after debt accrues over time, the liquidate() function will always fail.

It can not be liquidated because the D3VaultLiquidation.liquidate() function contains checks that the collateral token needs to have positive value and debt token needs to have negative value in the pool:

https://github.com/sherlock-audit/2023-06-dodo/blob/a8d30e611acc9762029f8756d6a5b81825faf348/new-dodo-v3/contracts/DODOV3MM/D3Vault/D3VaultLiquidation.sol#L42-L43
        require(isPositiveNetWorthAsset(pool, collateral), Errors.INVALID_COLLATERAL_TOKEN);
        require(!isPositiveNetWorthAsset(pool, debt), Errors.INVALID_DEBT_TOKEN);
These statements can not pass if the collateral token is the same as the debt token.

Extra info: Upon testing, I have found even another blocker to this liquidation scenario. Because of the way collateral ratios are calculated, the pool actually goes from a state of not liquidateable straight to bad debt. This causes an even previous liquidation check to fail:

https://github.com/sherlock-audit/2023-06-dodo/blob/a8d30e611acc9762029f8756d6a5b81825faf348/new-dodo-v3/contracts/DODOV3MM/D3Vault/D3VaultLiquidation.sol#L40
        require(!checkBadDebtAfterAccrue(pool), Errors.HAS_BAD_DEBT);
Conclusion and POC After much testing, I have verified that single-token pools cannot be liquidated. However, it turned out that the previous check regarding bad debt caused the revert. This is because the collateral ratio goes from a massively positive value directly to 0 when the pool becomes liquidateable. I played with the following vm.warp() value in the POC for a long time. For a warp value smaller than the one below, the d3MM.checkCanBeLiquidated(); returns false. Once the warp value hits around the value provided, the result is true. However, actually attempting the liquidate() call returns D3VAULT_HAS_BAD_DEBT.

The following is the POC. It's the exact same as the Dodo-provided D3Funding.t.sol.testCanBeLiquidated(), but I have changed the warp time to be closer to the time at which the liquidation flips from false to true, AND I have added the actual call to liquidate() to demonstrate the failure.

IF you remove the require(!checkBadDebtAfterAccrue(pool), Errors.HAS_BAD_DEBT); check, then my original comment passes as intended. The test will return the Errors.INVALID_COLLATERAL_TOKEN) error.
/*

    Copyright 2023 DODO ZOO.
    SPDX-License-Identifier: Apache-2.0

*/

pragma solidity 0.8.16;

import "forge-std/Test.sol";
import "../../TestContext.t.sol";

//import {Errors} from "contracts/DODOV3MM/lib/Errors.sol";

contract D3FundingTest is TestContext {
    MockERC20 public tokenEx;
    MockChainlinkPriceFeed public tokenExChainLinkOracle;

    function setUp() public {
        contextBasic();
        setVaultAsset();
        mintPoolCreator();

        tokenEx = new MockERC20("TokenEx", "TKEx", 18);
        tokenExChainLinkOracle = new MockChainlinkPriceFeed("TokenEx/USD", 18);
        tokenExChainLinkOracle.feedData(24 * 1e18);
    }

    function testCanBeLiquidated() public {
        vm.startPrank(poolCreator);
        d3Proxy.makerDeposit(address(d3MM), address(token1), 41 * 1e8);
        d3MM.borrow(address(token1), 100 * 1e8);
        vm.stopPrank();
        logCollateralRatio(address(d3MM));

        // pass
        vm.warp(28153000);
        logCollateralRatio(address(d3MM));
        uint256 ratio = d3Vault.getCollateralRatio(address(d3MM));
        assertEq(ratio, 0);
        bool can = d3MM.checkCanBeLiquidated();
        assertEq(can, true);

        // Liquidation
        token1.mint(user2, 100 ether);
        vm.prank(user2);
        token1.approve(address(d3Vault), 100 ether);

        vm.prank(address(d3MM));
        token1.approve(address(d3Vault), type(uint256).max);

        vm.prank(user2);
        vm.expectRevert(bytes(Errors.D3VAULT_HAS_BAD_DEBT));
        d3Vault.liquidate(
            address(d3MM),
            address(token1),
            1 ether,
            address(token1),
            1 ether
        );
    }
}

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

hrishibhat commented 1 year ago

@fatherGoose1 Additional Sponsor comment:

In fact, it can be liquidated. The liquidator can use another token supported by a different vault as collateral to repay the debt of the liquidation pool. We think it's invalid

fatherGoose1 commented 1 year ago

@hrishibhat This response from the sponsor does not seem to make sense. In a liquidation execution, a liquidator transfers in the debt token and receives the pool's collateral token. What collateral will the liquidator be paid in in this example? Remembering that this report is specifically talking about pools that deposit token1 and borrow token1, this is the liquidation scenario:

Pool has negative net worth of the debt token
Pool has no other collateral token to pay a liquidator

There is no way to make these checks pass with a single token pool.

        require(isPositiveNetWorthAsset(pool, collateral), Errors.INVALID_COLLATERAL_TOKEN);
        require(!isPositiveNetWorthAsset(pool, debt), Errors.INVALID_DEBT_TOKEN);
        require(getPositiveNetWorthAsset(pool, collateral) >= collateralAmount, Errors.COLLATERAL_AMOUNT_EXCEED);

Maybe Dodo can comment directly based on this info.

hrishibhat commented 1 year ago

@Attens1423 @traceurl

Attens1423 commented 1 year ago

If bad debts occur, the dodo liquidation method should be used. Overall, we do not recognize the effectiveness of this issue. If the pool can repay the loan, the liquidate method can be called. If the pool cannot, then the dodo liquidation method should be followed.

hrishibhat commented 1 year ago

@fatherGoose1

fatherGoose1 commented 1 year ago

@Attens1423 @hrishibhat I understand that. You're confirming the issue and sounds like you're saying that you will need to call liquidateByDodo() when bad debt occurs. Because of the bug that I have highlighted, the normal liquidation method cannot be performed AND bad debt must occur to some degree. I do not see how this is not a bug that needs to be fixed. It will cause bad debt to accrue in your system.

fatherGoose1 commented 1 year ago

Same impact as #192, where warden stated:

"The issue with this is that once this check has been triggered, no other market participants besides DODO can liquidate this position. This creates a significant inefficiency in the market that can easily to real bad debt being created for the pool. This bad debt is harmful to both the pool MM, who could have been liquidated with remaining collateral, and also the vault LPs who directly pay for the bad debt"

And sponsor responded:

"We have discovered some hidden issues in the dodo liquidation process, and we agree to modify the check of bad debts."

@hrishibhat

traceurl commented 1 year ago

@fatherGoose1 The case happens when pool borrows same token as collateral, and after a long time passed, the accrued interest causes bad debt. Once bad debt happens, we will call liquidateByDODO, the vault will lose very small amount of interests, user's principal is still safe. We think it is acceptable.

fatherGoose1 commented 1 year ago

Again, Dodo confirms the issue but decides not to fix. The impact is still the exact same as #192 which is receiving an award. @hrishibhat

hrishibhat commented 1 year ago

@fatherGoose1 the comparison with #192 it may not be right here: In 192 there is an error in how the bad debt is calculated regardless of the token. showed bad debt when there isn't where impact could build over time. Here there is a bad debt accrued. This only in the case when there is only one token as collateral and borrow, which seems unlikely and the impact not significant as pointed above. Although unsure of the actual loss in both cases this is my understanding. Not sure if I've missed anything.

fatherGoose1 commented 1 year ago

Hey @hrishibhat, the impact is the same between both this submission and #192. 192 explains that the function to calculate bad debt is incorrect, resulting in scenarios where liquidators cannot call the liquidate function when they should be able to. My submission shows how liquidators cannot call the liquidate function when they should be able to (specifically with single token pools). In both reports, the result is bad debt accruing which is negative for the system.

traceurl commented 1 year ago

192 points out the the calculation of bad debt is incorrect, which obviously has serious impact: some pools will be falsely marked as bad debt.

hrishibhat commented 1 year ago

Result: Low Unique Considering this issue a low based on the comments above and below. Additional Sponsor comment:

In the case of single-coin pools, the probability is relatively low. Moreover, due to the protection provided by borrowRate checks, even if Dodo liquidation is involved, only a very small portion of interest would be lost. In the worst-case scenario, it would be a negligible fraction of the principal. We consider this impact to be insignificant.

For the original code, #192 has a greater impact. Although this issue is not consistent with #192, its effect is minimal. We believe that adding an additional method for such edge cases would not be cost-effective and would increase code complexity. Furthermore, as mentioned in our initial response, even if we were to introduce a special method for single-coin liquidation, since there is no profit incentive for liquidation bots in the market, ultimately Dodo's liquidation will prevail.

It is likely to have a pool with only single token borrow + collateral, but it is not likely that the pool owner doesn’t do anything and wait for the interests to increase for a long time and in the end make the pool under liquidation.

sherlock-admin2 commented 1 year ago

Escalations have been resolved successfully!

Escalation status:

fatherGoose1: rejected

sherlock-audit / 2023-06-dodo-judging

0xDjango - Pools that borrow same token as collateral can't be liquidated #173

Pools that borrow same token as collateral can't be liquidated

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation