Minting credit tokens before creditMultiplier is updated down and redeeming for underlying tokens after creditMultiplier is updated down would cause an underlying token amount that is a part of pegTokenBalance to always remain in SimplePSM contract and be lost

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/governance/ProfitManager.sol#L79-L87
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/SimplePSM.sol#L103-L112
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/SimplePSM.sol#L87-L93
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/SimplePSM.sol#L134-L144

Vulnerability details

Impact

As mentioned by the following code comments for creditMultiplier, creditMultiplier can only be updated down.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/governance/ProfitManager.sol#L79-L87

    /// @notice multiplier for CREDIT value in the system.
    /// e.g. a value of 0.7e18 would mean that CREDIT has been discounted by 30% so far in the system,
    /// and that all lending terms will allow 1/0.7=1.42 times more CREDIT to be borrowed per collateral
    /// tokens, and all active debts are also affected by this multiplier during the update (e.g. if an
    /// address owed 1000 CREDIT in an active loan, they now owe 1428 CREDIT).
    /// The CREDIT multiplier can only go down (CREDIT can only lose value over time, when bad debt
    /// is created in the system). To make CREDIT a valuable asset to hold, profits generated by the system
    /// shall be redistributed to holders through a savings rate or another mechanism.
    uint256 public creditMultiplier = 1e18;

Before creditMultiplier is updated down, a user can call the following SimplePSM.mint function to mint some credit tokens by sending some underlying tokens, which are tracked by pegTokenBalance, to the SimplePSM contract. When only one user has minted the credit tokens, pegTokenBalance would equal the sent underlying token amount.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/SimplePSM.sol#L103-L112

    function mint(
        address to,
        uint256 amountIn
    ) external whenNotPaused returns (uint256 amountOut) {
        amountOut = getMintAmountOut(amountIn);
        pegTokenBalance += amountIn;
        ERC20(pegToken).safeTransferFrom(msg.sender, address(this), amountIn);
        CreditToken(credit).mint(to, amountOut);
        emit Mint(block.timestamp, to, amountIn, amountOut);
    }

For the same amountIn, the value returned by the following SimplePSM.getRedeemAmountOut function after creditMultiplier is updated down would be smaller than the value returned by the same function before creditMultiplier is updated down.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/SimplePSM.sol#L87-L93

    function getRedeemAmountOut(
        uint256 amountIn
    ) public view returns (uint256) {
        uint256 creditMultiplier = ProfitManager(profitManager)
            .creditMultiplier();
        return (amountIn * creditMultiplier) / 1e18 / decimalCorrection;
    }

Hence, after creditMultiplier is updated down, when the same user calls the following SimplePSM.redeem function with amountIn being the amountOut that was returned by the previous SimplePSM.mint function call, the amountOut that equals getRedeemAmountOut(amountIn) would be smaller than pegTokenBalance if there still is only one user who has minted credit tokens. In this case, pegTokenBalance -= amountOut would not reduce pegTokenBalance to 0, and ERC20(pegToken).safeTransfer(to, amountOut) would not transfer all of the pegTokenBalance underlying tokens out from the SimplePSM contract.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/SimplePSM.sol#L134-L144

    function redeem(
        address to,
        uint256 amountIn
    ) external returns (uint256 amountOut) {
        require(!redemptionsPaused, "SimplePSM: redemptions paused");
        amountOut = getRedeemAmountOut(amountIn);
        CreditToken(credit).burnFrom(msg.sender, amountIn);
        pegTokenBalance -= amountOut;
        ERC20(pegToken).safeTransfer(to, amountOut);
        emit Redeem(block.timestamp, to, amountIn, amountOut);
    }

Since creditMultiplier can only be updated down and there are no other functions besides the SimplePSM.redeem function for transferring the underlying tokens out from the SimplePSM contract, the underlying token amount that is pegTokenBalance - amountOut will remain in the SimplePSM contract forever and hence is lost. When multiple users mint the credit tokens before creditMultiplier is updated down and redeem for the underlying tokens after creditMultiplier is updated down, the same issue also exists in which an underlying token amount that is a part of pegTokenBalance would always remain in the SimplePSM contract and be lost.

Proof of Concept

The following steps can occur.

1. creditMultiplier is 1e18 at this moment, and Alice calls the SimplePSM.mint function to mint 100e18 credit tokens by sending 100e18 underlying tokens to the SimplePSM contract.
2. creditMultiplier is updated down to 0.7e18.
3. Alice calls the SimplePSM.redeem function with amountIn being 100e18 credit tokens to receive 70e18 underlying tokens so 30e18 underlying tokens remain in the SimplePSM contract.
4. Because creditMultiplier can only be updated down and there are no other functions besides the SimplePSM.redeem function for transferring the underlying tokens out from the SimplePSM contract, the 30e18 underlying tokens would remain in the SimplePSM contract forever and be lost.

Tools Used

Manual Review

Recommended Mitigation Steps

In the SimplePSM contract, an accounting state variable can be added to track the underlying token amount, which is a part of pegTokenBalance, that would remain in the SimplePSM contract due to the creditMultiplier reductions; then, a function that is only callable by an authorized role, such as coreRoles.GOVERNOR, can also be added for transferring such accounting state variable amount of the underlying token out from the SimplePSM contract.

Assessed type

Timing

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[eswak]

Disputing the validity of this issue : a PoC would be useful here to demonstrate the faulty behavior, but my intuition is that the sequence of events to reach the described state can't happen. In step 2, for the creditMultiplier to be updated to 0.7, there would need to be 30% of the supply not minted through the PSM and that defaults on a loan. So in step 3, Alice still has 100 credit tokens and redeem them for 70 pegTokens, but the borrower also has ~42.85 credit tokens (borrowed from the loan that defaulted), and these 42.85 credit tokens can be redeemed for the 30 pegTokens left in the PSM, and the PSM does not end with leftover funds.

There is actually a bug reported elsewhere in the creditMultiplier update (#292) that can cause non-zero PSM balance at the end of system wind down, but I don't think this issue is a duplicate because it doesn't hint to the root cause of the problem.

[c4-sponsor]

eswak (sponsor) disputed

[c4-judge]

Trumpero marked the issue as unsatisfactory:
Invalid