Ethereum Credit Guild - btk's results

Lines of code

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

Vulnerability details

Overview

SurplusGuildMinter.getReward() is checking if a user was slashed before loading the state.

Impact

The current implementation of getRewards() checks for slashes before loading the user's state from storage, causing loss of staked credit and guild rewards.

Proof of Concept

Vulnerability Details

Here is the getReward() function implementation:

    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
        )
    {
        bool updateState;
        lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
        if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
            slashed = true;
        }

It checks if a user was slashed before loading the state from storage. This results in a default value (0) for userStake.lastGaugeLoss, causing loss of staked credit and guild rewards if there was ever a prior loss for that term before the user staked.

Here is a coded PoC to demonstrate the issue:

    function testBreakGetRewards() public {
        credit.mint(address(this), 100e18);
        credit.approve(address(sgm), 100e18);
        sgm.stake(term, 100e18);

        vm.prank(governor);
        profitManager.setProfitSharingConfig(
            0.5e18, // surplusBufferSplit
            0, // creditSplit
            0.5e18, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );

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

        profitManager.notifyPnL(term, -50e18);

        guild.applyGaugeLoss(term, address(sgm));

        // For simplicity, assume the term was offboarded and re-onboarded again
        // After 1 week, the term was profitable again, so user's will start staking

        vm.warp(block.timestamp + 1 weeks);

        address randomUser = makeAddr("randomUser");
        credit.mint(randomUser, 100e18);

        vm.startPrank(randomUser);
        credit.approve(address(sgm), 100e18);
        sgm.stake(term, 100e18);
        vm.stopPrank();

        console.log("User State Before Get Reward Is Called: ");
        console.log("");

        console.log("User credit     :", sgm.getUserStake(randomUser, term).credit);
        console.log("User guild      :", sgm.getUserStake(randomUser, term).guild);
        console.log("User last loss  :", sgm.getUserStake(randomUser, term).lastGaugeLoss);
        console.log("User stake time :", sgm.getUserStake(randomUser, term).stakeTime);
        console.log("");

        sgm.getRewards(randomUser, term);

        console.log("User State After Get Reward Is Called: ");
        console.log("");

        console.log("User credit     :", sgm.getUserStake(randomUser, term).credit);
        console.log("User guild      :", sgm.getUserStake(randomUser, term).guild);
        console.log("User last loss  :", sgm.getUserStake(randomUser, term).lastGaugeLoss);
        console.log("User stake time :", sgm.getUserStake(randomUser, term).stakeTime);
    }

Logs result:

  User State Before Get Reward Is Called:

  User credit     : 100000000000000000000
  User guild      : 200000000000000000000
  User last loss  : 1679067880
  User stake time : 1679672680

  User State After Get Reward Is Called:
  
  User credit     : 0
  User guild      : 0
  User last loss  : 0
  User stake time : 0

Test Setup:

• Incorporate the tests in SurplusGuildMinterUnitTest
• Execute: forge test --mt testBreakGetRewards -vvv

Tools Used

Manual review

Recommended Mitigation Steps

A simple fix will be to load the state from storage before the check, as follow:

    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);

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/GuildToken.sol#L41

Vulnerability details

Overview

In the current design, the intention is to deploy multiple ProfitManagers (according to devs instructions). However, the guild token can only interact with one ProfitManager at a time.

Impact

Mutiple ProfitManagers cannot interact with the guild token. Specifically, any attempt to call the Guild::notifyGaugeLoss() function will revert.

Proof of Concept

The guild token's code reveals its exclusive support for a single manager:

    address public profitManager;

    function notifyGaugeLoss(address gauge) external {
        require(msg.sender == profitManager, "UNAUTHORIZED");

        lastGaugeLoss[gauge] = block.timestamp;
    }

External calls made by the guild token to claim rewards in the manager further emphasize this constraint:

    function _decrementGaugeWeight(
        address user,
        address gauge,
        uint256 weight
    ) internal override {
        ProfitManager(profitManager).claimGaugeRewards(user, gauge);
        /*
            .... unnecessary lines
        */
    }

    function _incrementGaugeWeight(
        address user,
        address gauge,
        uint256 weight
    ) internal override {
        ProfitManager(profitManager).claimGaugeRewards(user, gauge);
        /*
            .... unnecessary lines
        */
    }

This inability to interact with other managers limits the protocol's ability to add new managers to the system.

Here is a coded PoC to demonstrate the issue:

    function testMutipleProfitManagers() public {
        vm.startPrank(governor);
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(this));
        core.grantRole(CoreRoles.CREDIT_MINTER, address(this));
        vm.stopPrank();

        ProfitManager profitManager2 = new ProfitManager(address(core));
        vm.prank(governor);
        profitManager2.initializeReferences(address(credit), address(guild), address(psm));

        // Tx will succeed
        profitManager.notifyPnL(address(this), -30e18); 

        // Tx will revert
        vm.expectRevert("UNAUTHORIZED");
        profitManager2.notifyPnL(address(this), -30e18); 
    }

Test result:

[PASS] testMutipleProfitManagers() (gas: 2599377)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.64ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Test Setup:

• Incorporate the tests in Contract name
• Execute: forge test --mc contract-name --mt test-name -vvv

Tools Used

Manual review

Recommended Mitigation Steps

While the fix is challenging due to the guild token's need to call the manager's claimGaugeRewards() function, a potential solution may involve introducing a mapping of managers:

    // ProfileManager => bool
    mapping(address => bool) isManager;

    function notifyGaugeLoss(address gauge) external {
        require(isManager[msg.sender], "UNAUTHORIZED");

        lastGaugeLoss[gauge] = block.timestamp;
    }

Users can then pass the manager as follows:

    function _decrementGaugeWeight(
        address user,
        address gauge,
        address manager,
        uint256 weight
    ) internal override {
        require(isManager[manager], "Guild: Invalid Manager");
        ProfitManager(profitManager).claimGaugeRewards(user, gauge);
        /*
            .... unnecessary lines
        */
    }

    function _incrementGaugeWeight(
        address user,
        address gauge,
        address manager,
        uint256 weight
    ) internal override {
        require(isManager[manager], "Guild: Invalid Manager");
        ProfitManager(profitManager).claimGaugeRewards(user, gauge);
        /*
            .... unnecessary lines
        */
    }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/ERC20RebaseDistributor.sol#L338

Vulnerability details

Overview

The CreditToken::distribute() function is susceptible to griefing attacks, allowing malicious users to disrupt the rewards distribution timeline.

Impact

Malicious users can grief CreditToken rewards distribution, resulting in:

• Malicious users can exploit the vulnerability to tamper with the endTimestamp, delaying the reward distribution.
• This contradicts the intended gradual distribution over the specified period, potentially impacting the token holders' expectations.
• Delayed distribution might create frustration among users expecting timely rewards.

Proof of Concept

Vulnerability Details

In the CreditToken.distribute() function:

• The endTimestamp is updated based on the current block timestamp and a specified distribution period (DISTRIBUTION_PERIOD).
• This timestamp is crucial for subsequent calculations related to rebasing, ensuring a gradual reward distribution over the specified period.

Simple Scenario:

• Alice holds 10 credit tokens.
• An honest user triggers distribute() with 10 credits, meant to be distributed linearly over 30 days.
• After 15 days, 5 credits have been rebased.
• A malicious user, Bob, spams distribute() with 1 wei.
• The endTimestamp is updated, causing all pending rewards (not yet rebased) to be distributed over the next 30 days instead of the remaining 15.

This can occur without a griefing attack as credit::distribute() is called whenever a profit is realized.

            if (amountForCredit != 0) {
                CreditToken(_credit).distribute(amountForCredit);
            }

Here is a coded PoC to demonstrate the issue:

    function testSpamDistribute() public {
        vm.startPrank(governor);
        core.grantRole(CoreRoles.CREDIT_MINTER, address(this));
        core.grantRole(CoreRoles.CREDIT_REBASE_PARAMETERS, address(this));
        vm.stopPrank();

        // initial state
        token.mint(alice, 10e18);
        token.forceEnterRebase(alice);

        token.mint(address(this), 10e18);
        token.approve(address(token), 10e18);
        token.distribute(10e18);

        console.log("Start: ");
        console.log("Alice balance      :", token.balanceOf(alice));

        vm.warp(block.timestamp + 15 days);

        console.log("");
        console.log("Day 15: ");
        console.log("Alice balance      :", token.balanceOf(alice));

        token.mint(address(this), 1 wei);
        token.approve(address(token), 1 wei);
        token.distribute(1 wei);

        vm.warp(block.timestamp + 15 days);

        console.log("");
        console.log("Day 30: ");
        console.log("Alice balance      :", token.balanceOf(alice));
    }

Logs result:

  Start:
  Alice balance      : 10000000000000000000

  Day 15:
  Alice balance      : 15000000000000000000

  Day 30:
  Alice balance      : 17500000000000000000

Test Setup:

• Incorporate the tests in CreditTokenUnitTest
• Execute: forge test --mt testSpamDistribute -vvv

Tools Used

Manual review

Recommended Mitigation Steps

Since credit::distribute() is called whenever a profit is realized, I am unable to give any suggestion as big parts of the protocol need to be re-done. I can only point out the root cause of the problem, which is updating endTimestamp even when there is pending rewards (not yet rebased):

            // update rebasingSharePrice interpolation
            uint256 endTimestamp = block.timestamp + DISTRIBUTION_PERIOD;

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L331

Vulnerability details

Overview

The creditMultiplier impacts the value of CREDIT throughout the system. A reduction in this multiplier, results in a decreased CREDIT value across the system. For instance, a creditMultiplier of 0.7e18 implies a 30% discount on CREDIT, affecting borrowing terms and active debts proportionately.

The credit token provides two functions: totalSupply() represents the total existing tokens, and targetTotalSupply() represents the target after rebase reward interpolations. The current usage of credit.totalSupply() when calculating creditMultiplier is incorrect. Switching to credit.targetTotalSupply() is essential for accurate calculations.

Impact

Incorrect Credit Value: The creditMultiplier directly influences the value of CREDIT in the system. Using the incorrect method will lead to an inaccurate representation of the CREDIT value.

Inaccurate Debt Calculation: Active debts in the system are also influenced by the creditMultiplier. Employing the incorrect method will lead to inaccurate calculations of debts, affecting users who have outstanding loans denominated in CREDIT.

Unintended Distribution Discrepancies: The credit token is designed for fair reward distribution. Incorrectly calculating the creditMultiplier can lead to unintended discrepancies in reward distribution among token holders, impacting the economic incentives of the protocol.

[!NOTE]
Sponsor confirmed: "Confirming it should be targetTotalSupply."

Proof of Concept

Vulnerability Details

The current formula uses totalSupply(), which exclude unminted rebase rewards:

However, using targetTotalSupply() will accurately represent the target total number of tokens after rebase reward interpolations:

Here is a coded PoC to demonstrate the issue:

    function testInvalidCreditMultiplier() public {
        vm.startPrank(governor);
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(this));
        core.grantRole(CoreRoles.CREDIT_MINTER, address(this));
        vm.stopPrank();

        // initial state
        // 100 CREDIT circulating
        assertEq(profitManager.creditMultiplier(), 1e18);
        assertEq(credit.totalSupply(), 100e18);

        credit.mint(address(this), 100e18);
        credit.distribute(100e18);

        profitManager.notifyPnL(address(this), -30e18); 
        
        /* 
            creditMultiplier using totalSupply       : 0.70e18
            creditMultiplier using targetTotalSupply : 0.85e18
        */

        // Use targetTotalSupply() in notifyPnL() to see the diff
        console.log("Credit multiplier: ", profitManager.creditMultiplier());
    }

Logs result:

Credit Multiplier using totalSupply       : 0.70e18
Credit Multiplier using targetTotalSupply : 0.85e18

Test Setup:

• Incorporate the tests in ProfitManagerUnitTest
• Execute: forge test --mt testInvalidCreditMultiplier -vvv

Tools Used

Manual review

Recommended Mitigation Steps

To address this issue, update the creditMultiplier calculation as follows:

                // update the CREDIT multiplier
                uint256 creditTargetSupply = CreditToken(_credit).targetTotalSupply();
                uint256 newCreditMultiplier = (creditMultiplier *
                    (creditTargetSupply - loss)) / creditTargetSupply;
                creditMultiplier = newCreditMultiplier;

Assessed type

Other