search

code-423n4 / 2023-12-ethereumcreditguild-findings

17 stars 11 forks source link

ProfitManager's "creditMultiplier" calculation does not count undistributed rewards; this can cause value losses to users #292

Open c4-bot-1 opened 11 months ago

c4-bot-1 commented 11 months ago

Lines of code

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

Vulnerability details

The function notifyPnL in ProfitManager is called by LendingTerms when a loan is closed to report a profit or loss.

In case there is a loss, the amount in excess of the protocol's buffer is attributed to the credit token holders: this is done by slashing the creditMultiplier with the effect of depreciating the credit token relatively to the collateral.

The math for slashing the creditMultiplier is the following:

File: ProfitManager.sol
292:     function notifyPnL(
293:         address gauge,
294:         int256 amount
295:     ) external onlyCoreRole(CoreRoles.GAUGE_PNL_NOTIFIER) {
---
330:                 // update the CREDIT multiplier
331:                 uint256 creditTotalSupply = CreditToken(_credit).totalSupply();
332:                 uint256 newCreditMultiplier = (creditMultiplier *
333:                     (creditTotalSupply - loss)) / creditTotalSupply;
334:                 creditMultiplier = newCreditMultiplier;

It is particularly interesting that totalSupply() is used, since in a similar high-level accounting calculation, totalBorrowedCredit(), targetTotalSupply() is used instead:

File: ProfitManager.sol
172:     function totalBorrowedCredit() external view returns (uint256) {
173:         return
174:             CreditToken(credit).targetTotalSupply() -
175:             SimplePSM(psm).redeemableCredit();
176:     }

The usage of totalSupply() here can be problematic when a significant portion of the supply is in undistributed rewards.

In this case, the creditMultiplier slashing will be higher than it should because totalSupply() will return a much lower value than targetTotalSupply().

Impact

creditMultiplier slashing will always be higher than it should for correct accounting, penalizing credit token holders, and finally locking value in the protocol.

The negative effects will be proportional to the relative supply of credit tokens in undistributed rewards versus the interpolated supply of the credit token.

Proof of Concept

The following PoC in Foundry (full setup here) shows how an overshot creditMultiplier slashing locks collateral value in SimplePSM, with a net loss for protocol users, in a real-wold scenario:

    function testH2() external {
        uint ts = block.timestamp;

        // Set ProfitManager to 100% rewards for rebasing users
        pm.setProfitSharingConfig(
            0,          // surplusBufferSplit,
            1e18,       // creditSplit,
            0,          // guildSplit,
            0,          // otherSplit,
            address(0)  // otherRecipient
        );

        // User 1 deposit 3000 USDC in PSM, gets 3000 gUSDC, enters rebase
        address user1 = address(1);
        vm.startPrank(user1);

        coll.mint(user1, 3_000e18);
        coll.approve(address(sPsm), 3_000e18);
        sPsm.mintAndEnterRebase(3_000e18);

        // User 2 open Loan A, 1000 gUSDC, redeems for 1000 USDC
        address user2 = address(2);
        vm.startPrank(user2);

        coll.mint(user2, 1_000e18);
        coll.approve(address(lt), 1_000e18);
        bytes32 loanA = lt.borrow(1_000e18, 1_000e18);

        ct.approve(address(sPsm), 1_000e18);
        sPsm.redeem(user2, 1_000e18);

        // User 3 open Loan B, 1000 gUSDC, redeems for 1000 USDC
        address user3 = address(3);
        vm.startPrank(user3);

        coll.mint(user3, 1_000e18);
        coll.approve(address(lt), 1_000e18);
        bytes32 loanB = lt.borrow(1_000e18, 1_000e18);

        ct.approve(address(sPsm), 1_000e18);
        sPsm.redeem(user3, 1_000e18);

        // User 4 open Loan C, 1000 gUSDC, redeems for 1000 USDC
        address user4 = address(4);
        vm.startPrank(user4);

        coll.mint(user4, 1_000e18);
        coll.approve(address(lt), 1_000e18);
        bytes32 loanC = lt.borrow(1_000e18, 1_000e18);

        ct.approve(address(sPsm), 1_000e18);
        sPsm.redeem(user4, 1_000e18);

        // Time passes, all loans accrue 50% interest, loan B gets called
        ts += lt.YEAR() - 3 weeks;
        vm.warp(ts);
        lt.call(loanB);
        ts += 3 weeks;
        vm.warp(ts);

        // User 2 deposit 1500 USDC in PSM, gets 1500 gUSDC, and repay Loan A (500 profit) -> 1500 USDC in PSM
        vm.startPrank(user2);
        coll.mint(user2, 500e18);
        coll.approve(address(sPsm), 1500e18);
        sPsm.mint(user2, 1500e18);

        ct.approve(address(lt), 1500e18);
        lt.repay(loanA);

        // Now User 1's 3000 gUSDC balance is interpolating towards 3500 gUSDC
        assertEq(3_000e18, ct.totalSupply());
        assertEq(ct.totalSupply(), ct.balanceOf(user1));
        assertEq(3_500e18, ct.targetTotalSupply());

        // ---  Everything good till here; now we get to the bug:
        // User 3 completely defaults on Loan B, 1000 gUSDC loss is reported, 
        // creditMultiplier becomes 1e18 * (3000 - 1000) / 3000 = 0.6667e18
        // 🚨 if targetTotalSupply was used, this would be 1e18 * (3500 - 1000) / 3500 = 0.714285e18
        ah.forgive(loanB);
        assertApproxEqRel(pm.creditMultiplier(), 0.6667e18, 0.0001e18 /* 0.01% */);

        // User 4's Loan C now owes 1500 / 0.66667 = 2250 gUSDC
        uint loanCdebt = lt.getLoanDebt(loanC);
        assertApproxEqRel(loanCdebt, 2250e18, 0.0001e18 /* 0.01% */);

        // User 4 deposit 1500 USDC in PSM, gets 2250 gUSDC, and repay Loan C (750 profit) -> 3000 USDC in PSM
        vm.startPrank(user4);
        coll.mint(user4, 500e18);
        coll.approve(address(sPsm), 1500e18);
        sPsm.mint(user4, 1500e18);

        ct.approve(address(lt), loanCdebt);
        lt.repay(loanC);

        // Now User 1's 3000 gUSDC balance is interpolating towards 4250
        assertEq(3_000e18, ct.totalSupply());
        assertEq(ct.totalSupply(), ct.balanceOf(user1));
        assertApproxEqRel(4_250e18, ct.targetTotalSupply(), 0.0001e18 /* 0.01% */);

        // User 1 waits for the interpolation to end
        ts += ct.DISTRIBUTION_PERIOD();
        vm.warp(ts);

        // User 1 redeems 4250 gUSDC for 4250 * 0.66667 = 2833 USDC -> 167 USDC in PSM (🚨 there should be no leftover)
        vm.startPrank(user1);
        ct.approve(address(sPsm), ct.balanceOf(user1));
        sPsm.redeem(user1, ct.balanceOf(user1));

        assertApproxEqRel(2833.3e18, coll.balanceOf(user1), 0.0001e18 /* 0.01% */);

        // 🚨 this value remains locked in the SimplePSM contract as a result of the incorrect accounting
        assertApproxEqRel(166.66e18, coll.balanceOf(address(sPsm)), 0.0001e18 /* 0.01% */);

        // ℹ️ if ProfitManager used targetTotalSupply, the value locked would be ~2e4 lost to rounding
    }

Tools Used

Code review, Foundry

Recommended Mitigation Steps

Consider using targetTotalSupply() instead of totalSupply() in the creditMultiplier slashing calculation:

    // ProfitManager:330
    // update the CREDIT multiplier
-   uint256 creditTotalSupply = CreditToken(_credit).totalSupply();
+   uint256 creditTotalSupply = CreditToken(_credit).targetTotalSupply();
    uint256 newCreditMultiplier = (creditMultiplier *
        (creditTotalSupply - loss)) / creditTotalSupply;
    creditMultiplier = newCreditMultiplier;
    emit CreditMultiplierUpdate(
        block.timestamp,
        newCreditMultiplier
    );

Assessed type

Math

c4-pre-sort commented 11 months ago

0xSorryNotSorry marked the issue as sufficient quality report

c4-pre-sort commented 11 months ago

0xSorryNotSorry marked the issue as primary issue

c4-pre-sort commented 11 months ago

0xSorryNotSorry marked the issue as high quality report

0xSorryNotSorry commented 11 months ago

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

eswak commented 11 months ago

Confirming this issue, and thanks a lot for the quality of the report!

I wonder if this should be qualified as "Medium", as duplicates of this issue are qualified as Medium, and even though the protocol could end up with funds left in the PSM, all users are treated the same way, and there is always the possibility to recover the funds and organize and airdrop / multisend.

c4-sponsor commented 11 months ago

eswak (sponsor) confirmed

c4-sponsor commented 10 months ago

eswak marked the issue as disagree with severity

c4-judge commented 10 months ago

Trumpero changed the severity to 2 (Med Risk)

c4-judge commented 10 months ago

Trumpero marked the issue as satisfactory

c4-judge commented 10 months ago

Trumpero marked the issue as selected for report

0xbtk commented 10 months ago

Hey @Trumpero, I believe that this issue should be a high because 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. Note that creditMultiplier can only decrease, and due to the current logical error, it's diminishing too quickly, resulting in direct losses for borrowers and holders.

RunSoul22 commented 10 months ago

I agree with @0xbtk that this issue should be a high.

stalinMacias commented 10 months ago

@eswak Could you take a second look at this? Isn't the CreditToken.totalSupply() return the Total number of the tokens in existence.. If a slashing event occurs, why would the creditMultiplier need to consider the future rewards (__unmintedRebaseRewards.targetValue), aren't those tokens locked until a date in the future? Why would the slashing consider those future rewards in the present? Suppose a slashing occurs on day 2 of the reward's distribution, why would the rewards from day 3 to day 30 need to be considered as part of the existing supply? is not the purpose of the distribution of rewards to lock rewards and as time passes, slowly unlock them to be part of the existing supply?

I think the value of the creditMultiplier is correctly calculated by using the amount of existing supply at the moment when the slashing event occurs. I reiterate again, why a supply that is meant to be unlocked in the future would need to be considered in the present?

I just want to emphasize something, the unmintedRewards are indeed considered when reading from CreditToken.totalSupply() Understanding that unmintedRewards are those rewards that have been unlocked based on the total time that has passed since they have been added to the distribution period. On the other hand, the future unminted rewards (__unmintedRebaseRewards.targetValue) are the ones that I believe should not be considered part of the supply in the present because those rewards are yet to be slowly added to the supply in the future, not in the present

stalinMacias commented 10 months ago

I've taken a second look at the report and the problem seems to be a legit issue, if the undistributed rewards are not included as part of the existing supply at the moment of the slashing that would bring down the creditMultiplier more than what it should if all the undistributed rewards are included, this would cause that the PSM module has more PeggedTokens than what they could be claimed for using the total CreditTokens in existence.

As for the severity, I think @eswak has a point in his comment (severity should be medium), all the funds that would've been left in the PSM could've been recovered and distributed to the users so they ended up equal as if the creditMultiplier would've been updated considering all the supply, I mean, the users would not lose their assets permanently, right? It would be a temporary loss while the extra funds are correctly distributed, it may take some extra work, but in the end, all users could get the exact amount of peggedTokens they could've claimed if the creditMultiplier had been updated correctly. No PeggedTokens would be lost in reality, there would only be more PeggedTokens in the PSM module than the total PeggedTokens that could be claimed for, thus, the extra tokens can be claimed by governance and distributed accordingly, making whole to all the users. tldr; no funds are lost, the extra tokens left in the PSM module, and they can be claimed by governance and distributed accordingly.

Plus, all new minting and borrowing, and basically everything, from that point onwards will use the new value of the creditMultiplier, even though it was brought down more than what it should, all the accounting from that point and beyond it will use the new value of the creditMultiplier

3docSec commented 10 months ago

Hey @stalinMacias the point of the credit multiplier update, and the goal of the math behind it, is to depreciate the synthetic versus the underlying so that:

This report proves how the second point is not respected, so value can remain locked in the protocol at the user’s expense. This should be clear in the PoC but you are very right that I should have point it out in the report.

@Trumpero A few wardens suggested this finding could be high and I tend to agree with that because the remediation of a governance action suggested by the sponsor may in practice not always be feasible in terms of:

stalinMacias commented 10 months ago

@3docSec For the points that you've just mentioned is exactly why this fits as a medium severity. Yes, it may take some extra work to governance for preparing the distribution of the extra PeggedTokens left in the PSM module. As for the second point, how much gas does it take to make an ERC20 transfer? Include it inside a batch of transfers, and how much would it cost to make full the user? Unless the user's difference was less than 1-2 usd then it might be more expensive to credit the difference, but even though, that is doable, that's the reason why this is fine as a medium severity, for a report to be classified as a high severity, there should be a permanent loss of funds, a way that it is impossible to recover them

0xbtk commented 10 months ago

Hey @stalinMacias, not all borrowers will redeem their credit in the PSM. Those holding credit during a loss will incur more losses than they should. Any thoughts on how governance can refund them?

Check out #484 for more info.

stalinMacias commented 10 months ago

@0xbtk Those incurred losses for the holders are exactly what it should be distributed by governance. As for the question of how could governance refund them, The SimplePSM contract inherits from the CoreRef contract, the CoreRef has the emergencyAction(), governance can simply craft a calldata to transfer the extra PeggedTokens that needs to be distributed to the holders who were impacted by the creditMultiplier going down more than what it should. So, governance transfers those PeggedTokens to an account of their own, they collect the information of how many PeggedTokens need to be sent to who to cover the difference and then they do those transfers in batch. That's it, governance recovered the extra tokens and distributed them.

That is because I consider this issue not to be a high severity. The tokens are not lost and they are not stuck forever in the PSM.

Trumpero commented 10 months ago

I consider this issue as medium because it doesn't cause a direct loss to the protocol or users. As the sponsor stated, the protocol could end up with funds left in the PSM, but all users are treated the same way, and Governor can still recover the funds by emergencyActions