Beedle - Oracle free perpetual lending

Summary

TWO ATTACK VECTORS POSSIBLE THAT INVOLVE THE FOLLOWING FUNCTIONS: startAuction() & buyLoan()

(In case my explanations are a bit hard to follow, please see my recommendations section below where I demonstrate how to mitigate against both attack vectors.)

Vulnerability Details

A) REENTRANCY:

This attack vector only requires loanId to be already in auction, and then calling the buyLoan() function:

• ERC20 transfer() triggered cross-contract reentrancy attack vector via callback into buyLoan(): for it to work, the attacker(msg.sender) must own/control the receiver address, and the receiver address must be a contract, and the transferred ERC20 token must enable callback functionality via the ERC20 transfer() function.

Different scenarios here:

-- if attacker is old pool's lender, he doesnt need to wait for another lender to buy his loanId in the auction, just calls the buyLoan() successfully and clears old pool's debt for loanId, while transferring the loan to whatever poolId he wishes that fits the loan parameters. He remains the lender of the loan, and DoS the new pool's lender for the loanId. Sort of griefing attack. Attacker can reenter the buyLoan() multiple times until just before it would revert due to the following line reverting due to underflow, making the griefing attack worse on the new pool & pool's lender, increasing the new pool's loan debt more than what is valid, messing up the balance mappings completely:

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

-- if attacker is not old pool's lender, then he benefits from griefing attack and messing with protocol/pool/Lender contract accounting, as well as becoming the new lender for the bought loanId, effectively DoS-ing the loan at least from new legit lender's management thereof perspective. Possibly in terms of borrower repayment too, didnt check this yet.

-- regardless of who the attacker is, it seems to be able to drain target pool's balance mapping as much as possible before reverting as per above outstandingLoans underflow and increasing its loans debt mapping balance, while simultaneously increasing the old pool's balance mapping and reducing its loans debt mapping balance, while causing fees to be sent to receiver/governance on each reentrancy.

B) CALLING buyLoan() FUNCTION MANUALLY MULTIPLE TIMES:

This attack vector involves calling startAuction() first then calling buyLoan():

Can either have each call use same loanId & poolId or same loanId but different poolId. Works either way.

Currently it seems this attack can only be mitigated/prevented if the timeElapsed is zero or too small, which will make the function revert at this line:

if (pools[poolId].interestRate > currentAuctionRate) revert RateTooHigh();

    uint256 timeElapsed = block.timestamp - loan.auctionStartTimestamp;
    uint256 currentAuctionRate = (MAX_INTEREST_RATE * timeElapsed) /
        loan.auctionLength;
    // validate the rate
    if (pools[poolId].interestRate > currentAuctionRate) revert RateTooHigh();

MAIN DIFFERENCES BETWEEN THESE TWO ATTACK VECTORS:

Main differences between the two attack vectors, i.e. between cross-contract reentrancy via callback and calling the buyLoan function multiple times, is as follows:

• with the reentrancy attack vector, the rogue lender/attacker doesnt need to call the startAuction() function each time, whereas with the multiple independent buyLoan() function calls the rogue lender/attacker needs to call startAuction() every time before calling buyLoan(), but this should work and be no problem for the attacker.

• otherwise they are mostly the same in terms of effect/damage/risk. Both attacks are limited by when the statement on L502 will revert, because it will revert at some point due to underflow:

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

So the above line is the primary limiting factor during both types of attacks.

Impact

DAMAGE:

• bad griefing attack against new legit lender & his pool, as well as against protocol, which causes the accounting of Lender contract and affected pool to be completely incorrect.

• borrower unaffected in most cases, except for maybe less/zero interest loan or worst case where loan potentially is DoS and cannot be repaid? need to confirm if this is possible.

• old lender seems unaffected, unless old lender IS attacker, i.e. also the new lender, who then benefits from clearing his pool from more debt than is valid.

• msg.sender of buyLoan doesnt need to be owner/lender of new pool(pool that bought the loan in auction), but becomes loan's new lender, effectively DoS-ing the loan for the new pool and legit lender.

• attacker, who's loan's new lender, can now repeat the process by auctioning off the loan again and calling buyLoan again soon enough after and before anyone else can buy the loan, to mess up the loan's current pool's accounting even further, increasing the griefing attack level/intensity, or just selecting a new pool to grief attack.

• every invalid(messes accounting) but successful call(doesnt revert) to buyLoan triggers another transfer of fees to receiver/governance, which adds to incorrect accounting of protocol

• rogue lender/attacker can either target same pool over & over, or target multiple pools in order to artificially clear his outstandingLoans balance, and end the attack just before underflow would trigger a revert.

Tools Used

VSC, manual, and lots of head scratching and losing my train of thoughts over and over. Hopefully my explanations were clear?

Recommendations

• Currently not following CEI pattern. At the very least the L517-L522 block should be moved to above the transfer function, which will effectively eliminate the callback reentrancy attack because it will revert at the following check:

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

• For the multiple function calls with same loanId and same/different poolId, the mitigation would be to ensure msg.sender of buyLoan() is not equal to current lender of loanId being auctioned, as well as a check to ensure that the msg.sender is equal to the lender of the new pool thats buying the auctioned loan, no exceptions.

• It should not be possible for receiver/governance address to be a lender address too.

Here is the function with the 3 recommended actions & checks implemented:

function buyLoan(uint256 loanId, bytes32 poolId) public {
    // get the loan info
    Loan memory loan = loans[loanId];
    // validate the loan
    if (loan.auctionStartTimestamp == type(uint256).max)
        revert AuctionNotStarted();
    if (block.timestamp > loan.auctionStartTimestamp + loan.auctionLength)
        revert AuctionEnded();
    if (msg.sender == loan.lender) revert Unauthorized(); 	/// @audit added check
    if (msg.sender != pools[poolId].lender) revert Unauthorized(); 	/// @audit added check
    // calculate the current interest rate
    uint256 timeElapsed = block.timestamp - loan.auctionStartTimestamp;
    uint256 currentAuctionRate = (MAX_INTEREST_RATE * timeElapsed) /
        loan.auctionLength;
    // validate the rate
    if (pools[poolId].interestRate > currentAuctionRate) revert RateTooHigh();
    // calculate the interest
    (uint256 lenderInterest, uint256 protocolInterest) = _calculateInterest(
        loan
    );

    // reject if the pool is not big enough
    uint256 totalDebt = loan.debt + lenderInterest + protocolInterest;
    if (pools[poolId].poolBalance < totalDebt) revert PoolTooSmall();

    // if they do have a big enough pool then transfer from their pool
    _updatePoolBalance(poolId, pools[poolId].poolBalance - totalDebt);
    pools[poolId].outstandingLoans += totalDebt;

    // now update the pool balance of the old lender
    bytes32 oldPoolId = getPoolId(
        loan.lender,
        loan.loanToken,
        loan.collateralToken
    );
    _updatePoolBalance(
        oldPoolId,
        pools[oldPoolId].poolBalance + loan.debt + lenderInterest   
    );
    pools[oldPoolId].outstandingLoans -= loan.debt;     

    // update the loan with the new info 	/// @audit moved this block before the transfer() function, in line with CEI
    loans[loanId].lender = msg.sender;
    loans[loanId].interestRate = pools[poolId].interestRate;
    loans[loanId].startTimestamp = block.timestamp;
    loans[loanId].auctionStartTimestamp = type(uint256).max;
    loans[loanId].debt = totalDebt;
    
    // transfer the protocol fee to the governance
    IERC20(loan.loanToken).transfer(feeReceiver, protocolInterest);     

    emit Repaid(
        loan.borrower,
        loan.lender,
        loanId,
        loan.debt + lenderInterest + protocolInterest,
        loan.collateral,
        loan.interestRate,
        loan.startTimestamp
    );

    emit Borrowed(
        loan.borrower,
        msg.sender,
        loanId,
        loans[loanId].debt,
        loans[loanId].collateral,
        pools[poolId].interestRate,
        block.timestamp
    );
    emit LoanBought(loanId);
}