code423n4 commented 1 year ago

Lines of code

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#L546-L549 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 https://github.com/code-423n4/2022-10-inverse#fixed-rates-market-protocol-overview 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 states that "a DOLA Fed mints DOLA to a market, which is then available to borrow for users holding DBR, using the Borrow function", users who do not own any DBR are still able to call the borrow function for borrowing DOLA against the deposited collateral. When this occurs, because the borrower has a DBR deficit, a replenisher could call the forceReplenish function to increase the borrower's debt. However, the borrower can monitor the replenisher's forceReplenish transaction in the mempool and front-runs it by sending a repayAndWithdraw transaction with a higher gas fee or by ordering such repayAndWithdraw transaction before such forceReplenish transaction if the borrower happens to be the relevant miner; after the front-running, the borrower would receive all of the deposited collateral. When the replenisher does not call the forceReplenish function promptly, such as that the replenisher might notice the borrower's DBR deficit after days or weeks have been passed since the borrower called the borrow function, the borrower is then able to borrow and utilize DOLA for these days or weeks while getting back all of the deposited collateral at the end without owning any DBR given the front-running is successful. Moreover, the DOLA amount that should be reserved for the DBR owners is borrowed by such borrower and becomes unavailable to the DBR owners, which is unfair to them.

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#L546-L549

    function repayAndWithdraw(uint repayAmount, uint withdrawAmount) public {
        repay(msg.sender, repayAmount);
        withdraw(withdrawAmount);
    }

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 testBorrowDOLAWhenHoldingNoDBR() public {
        gibWeth(user, wethTestAmount);

        // before borrowing, user holds no DBR or DOLA
        assertEq(dbr.balances(user), 0);
        assertEq(dbr.balanceOf(user), 0);
        assertEq(DOLA.balanceOf(user), 0);

        vm.startPrank(user);

        // user borrows wethTestAmount DOLA after depositing wethTestAmount WETH while holding no DBR
        deposit(wethTestAmount);
        market.borrow(wethTestAmount);

        vm.stopPrank();

        // after borrowing, user holds 0 WETH and wethTestAmount DOLA at this moment
        assertEq(WETH.balanceOf(user), 0);
        assertEq(DOLA.balanceOf(user), wethTestAmount);

        // After 1 week, replenisher notices that user did not own any DBR before borrowing and decides to force user to replenish DBR.
        // Yet, after noticing replenisher's forceReplenish transaction in the mempool,
        //   user would front-run it by sending a repayAndWithdraw transaction with a higher gas fee.
        vm.warp(block.timestamp + 1 weeks);

        // user has a DBR deficit at this moment
        uint256 dbrDeficit = dbr.deficitOf(user);
        assertGt(dbrDeficit, 0);

        vm.startPrank(user);

        // to perform the front-running, user sends the repayAndWithdraw transaction for getting back all of the deposited WETH 
        market.repayAndWithdraw(wethTestAmount, wethTestAmount);

        vm.stopPrank();

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

        // at the end, user gets back all of the deposited WETH while being able to utilize the borrowed DOLA for 1 week without holding any DBR
        assertEq(WETH.balanceOf(user), wethTestAmount);
        assertEq(DOLA.balanceOf(user), 0);
    }

Tools Used

VSCode

Recommended Mitigation Steps

The borrowInternal function can be updated to only allow the borrower to borrow the DOLA amount that corresponds to the borrower's DBR balance that is available for borrowing DOLA. If the borrower does not own any DBR, then calling this function should revert.

d3e4 commented 1 year ago

See my comment on #579.

c4-judge commented 1 year ago

0xean marked the issue as primary issue

c4-sponsor commented 1 year ago

08xmt marked the issue as sponsor disputed

08xmt commented 1 year ago

Being able to borrow DOLA with 0 DBR is intended behaviour. It allows for easy set-up of DOLA loans, where the loan is partially used to buy DBR tokens, without needing flashloan behaviour. The borrower will be paying the inflated borrowing APY that is set in the market. UI elements will be used to protect normal users from doing this.

Better wording could have been used to describe the behaviour of DBR, as DBR tokens give the right to borrow 1 DOLA without paying interest rates, where borrowing without DBR will make the user subject to high interest rates.

0xean commented 1 year ago

closing as invalid, see #579 for more info.