Ethereum Credit Guild - alexzoid's results

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SurplusGuildMinter.sol#L229

Vulnerability details

Impact

Due to the uninitialized userStake.lastGaugeLoss in the SurplusGuildMinter.getRewards() function, a new user's stake is consistently and erroneously slashed. This critical flaw not only imposes unjustified financial losses on users but also undermines the integrity of the staking mechanism.

Proof of Concept

1. Setting up the Term: Initialize a term with a non-zero lastGaugeLoss in the past and simulate the passage of time.
2. Initiating New User Stake: Create a new user stake and validate the stake's initialization and credit balance.
3. Observing the Erroneous Slashing: Advance the blockchain state again and observe the unwarranted slashing with getRewards().

Add the function below into test/unit/loan/SurplusGuildMinter.t.sol.

function testAuditUserAlwaysSlashed() public {
    
    //
    // 1. Setting up the Term (with non-zero `lastGaugeLoss` somewhere in the past)
    // 

    // simulate a past loss to test its impact on new stakers  
    testUnstakeWithLoss();

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

    // there was a loss somewhere in the past
    uint256 lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
    assertGt(lastGaugeLoss, 0); 
    assertGt(block.timestamp, lastGaugeLoss);

    //
    // 2. Initiating New User Stake 
    //

    // mint credit for a user
    address newStaker = vm.addr(1);
    credit.mint(newStaker, 100e18);

    // user don't have an active stake yet
    assertEq(sgm.getUserStake(newStaker, term).stakeTime, 0);

    // set up a new staker and confirm initial conditions
    vm.startPrank(newStaker);
    credit.approve(address(sgm), 100e18);
    sgm.stake(term, 100e18);
    assertEq(credit.balanceOf(newStaker), 0); 
    assertEq(sgm.getUserStake(newStaker, term).credit, 100e18);
    vm.stopPrank();

    // new stake `lastGaugeLoss` was initialized with term's `lastGaugeLoss` correctly
    lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
    assertEq(sgm.getUserStake(newStaker, term).lastGaugeLoss, lastGaugeLoss);

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

    //
    // 3. Observing the Erroneous Slashing
    //

    (, , bool slashed) = sgm.getRewards(newStaker, term);
    assertTrue(slashed); 
}

Run test with forge test -vv --match-test testAuditUserAlwaysSlashed. Output example:

[⠆] Compiling...
No files changed, compilation skipped

Running 1 test for test/unit/loan/SurplusGuildMinter.t.sol:SurplusGuildMinterUnitTest
[PASS] testAuditUserAlwaysSlashed() (gas: 1011790)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.07ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

It is recommended to initialize the userStake variable before its use in the getRewards() function.

/// @notice get rewards from a staking position without unstaking.
/// This can be used to slash users that have an outstanding unapplied loss.
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
    )
{
    userStake = _stakes[user][term];

    bool updateState;
    lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
    if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
        slashed = true;
    }

    // if the user is not staking, do nothing
    if (userStake.stakeTime == 0)
        return (lastGaugeLoss, userStake, 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);

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/GuildToken.sol#L197-L200 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SurplusGuildMinter.sol#L130-L136

Vulnerability details

Impact

The setProfitManager function in GuildToken allows for the changing of the ProfitManager address. However, this change is not reflected in other components like LendingTerm, SimplePSM, and SurplusGuildMinter, which still refer to the old address. This inconsistency can lead to a critical system failure. This discrepancy can result in failed transactions or incorrect reward distributions, leading to system instability and potential financial risks.

Proof of Concept

1. GuildToken.setProfitManager() is executed with newProfitManager.
2. SurplusGuildMinter.stake() makes a call to oldProfitManager.donateToTermSurplusBuffer().
3. Subsequently, there is a call to GuildToken.incrementGauge(), which incorrectly calls newProfitManager.claimGaugeRewards().
4. There is no functionality to update the ProfitManager address in SurplusGuildMinter as it is in GuildToken.

Tools Used

VSCode

Recommended Mitigation Steps

Ensure that the method of updating the ProfitManager address is consistent across all components (GuildToken, LendingTerm, SimplePSM, SurplusGuildMinter). This can be achieved by implementing a similar update mechanism in all components or by restricting the ability to change the ProfitManager in GuildToken to align with other components.

Assessed type

Other