The seizeLoan() function does not correctly check if the auction has ended before closing the debt

Severity

High Risk

Summary

The seizeLoan() function does not correctly check if the auction has ended before closing the debt, allowing the attacker to close the debt and take the collateral without paying the full amount owed.

Vulnerability Details

POC

    function test_Exploit_Size() public {
        console.log("borrowerBeforecollateralToken", collateralToken.balanceOf(borrower));
        console.log("borrowerBeforeloanToken", loanToken.balanceOf(borrower));

        console.log("lender1BeforecollateralToken", collateralToken.balanceOf(lender1));
        console.log("lender1BeforeloanToken", loanToken.balanceOf(lender1));

        console.log("lender2BeforecollateralToken", collateralToken.balanceOf(lender2));
        console.log("lender2BeforeloanToken", loanToken.balanceOf(lender2));
        console.log("=============================================================");
        // Préstamo y subasta iniciales

        test_startAuction();
        console.log("borrower_test_startAuctioncollateralToken",collateralToken.balanceOf(borrower));
        console.log( "borrower_test_startAuctionloanToken",loanToken.balanceOf(borrower));

        console.log("lender1_test_startAuctioncollateralToken",collateralToken.balanceOf(lender1) );
        console.log("lender1_test_startAuctionloanToken",loanToken.balanceOf(lender1));

        console.log("lender2_test_startAuctioncollateralToken",collateralToken.balanceOf(lender2));
        console.log("lender2_test_startAuctionloanToken",loanToken.balanceOf(lender2));
        console.log("=============================================================");

        // Array de ids
        uint256[] memory loanIds = new uint256[](1);
        loanIds[0] = 0;

        vm.warp(2 days);

        vm.startPrank(lender2);
        // Llamamos a seizeLoan
        lender.seizeLoan(loanIds);

        console.log("borrowerAftercollateralToken", collateralToken.balanceOf(borrower));
        console.log("borrowerAfterloanToken", loanToken.balanceOf(borrower));

        console.log("lender1AftercollateralToken", collateralToken.balanceOf(lender1));
        console.log("lender1AfterloanToken", loanToken.balanceOf(lender1));

        console.log("lende21AftercollateralToken", collateralToken.balanceOf(lender2));
        console.log("lende21AftercollateralTokenloanToken", loanToken.balanceOf(lender2));

        // El borrower debería conservar la deuda
        assertGt(loanToken.balanceOf(borrower), 0);

        // El lender no debería recibir el colateral

    }

EXPLANATION:

At the start, the borrower's loanToken balance and lender1's collateralToken balance is 0.
Then with test_startAuction(), the borrower receives 995000000000000000000000 loanTokens.
The time to end the auction is advanced. seizeLoan is called.
After seizeLoan, the borrower still has 9950000000000000000000000000 loanTokens. They were not taken away as they should be.
And lender1 received 9950000000000000000000000000 collateralTokens. They were transferred to him before validating.
Therefore, it is proven that seizeLoan:
Is not closing the borrower's debt as it should.
It is transferring the collateral to the lender before verifying the auction.

If the vulnerability in seizeLoan() did not exist, the expected behaviour should be:

At startup, the balances of the borrower and lender are 0.
With test_startAuction(), the borrower receives the loanTokens.
The auction ends.
SeizeLoan is called.
As there is no vulnerability, seizeLoan should:
Validate that the auction ended.
Close the debt: the borrower's loanTokens should go down to 0.
Transfer the collateral to the lender.

Therefore, after seizeLoan we should have:

Borrower's loanTokens balance = 0.
Lender's collateralTokens balance > 0.
In short, if there were no vulnerability:

The borrower would run out of loanTokens after seizeLoan. The lender would receive the collateral

Impact

There is a problem in the seizeLoan logic that can result in loss of funds for the lender.

Tools Used

Manual review and Foundry.

Recommendations

To fix the vulnerability in seizeLoan(), fund transfers should be moved to occur after all validations.

function seizeLoan(uint256[] calldata loanIds) public {

  for (uint i = 0; i < loanIds.length; i++) {

    uint256 loanId = loanIds[i];

    Loan memory loan = loans[loanId];

// Validaciones

    if (loan.auctionStartTimestamp == type(uint256).max)
      revert AuctionNotStarted();

    if (block.timestamp < loan.auctionStartTimestamp + loan.auctionLength)
      revert AuctionNotEnded();

// Cálculos

    uint256 govFee = (borrowerFee * loan.collateral) / 10000;

    bytes32 poolId = keccak256(abi.encode(loan.lender, loan.loanToken, loan.collateralToken));

// Transferencias de fondos

    IERC20(loan.loanToken).transfer(loan.lender, loan.debt);

    IERC20(loan.collateralToken).transfer(feeReceiver, govFee);

    IERC20(loan.collateralToken).transfer(loan.lender, loan.collateral - govFee);

    pools[poolId].outstandingLoans -= loan.debt;

    emit LoanSiezed(...);

    delete loans[loanId];

  }

}

In this way all validations are done first, then the calculations, and finally the token transfers.

Cyfrin / 2023-07-beedle