code-423n4 / 2024-02-spectra-findings

4 stars 2 forks source link

A momentary de-pegging can result in a loss for all PT holders, although the market recovers while the protocol could "earn" from the de-pegging. #107

Closed c4-bot-3 closed 8 months ago

c4-bot-3 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L901

Vulnerability details

Summary

The ptRate and ibtRate are maintained and updated at the PT contract level, and they are updated not systematically but rather only when any user interacts with the contract. Furthermore, ptRate is only allowed to decrease, which makes it unfavorable for all users in the event of any (momentary) past de-pegging event regardless of the current situation.

Vulnerability Details

In the protocol, PTs are supposed to be redeemable 1:1 with the underlying assets except when in the (continued) negative yield case. However, the ptRate is only allowed to decrease even if the market recovers from a (momentary) de-pegging event. Furthermore, the PT contract's ptRate and ibtRate are updated only when any user interacts with the protocol. This poses a risk for loss of asset balances for every user of the protocol if any user interacts with the protocol when ibtRate is decreased. Even if the market recovers (e.g., ibtRate increased back), the users will never redeem the fair amount of assets back, and all the loss of users will be taken as the "gain" of the protocol. This issue is especially concerning because the asset loss of all users can be caused by a single user/actor who interacts with the protocol.

Impact

The impact of this vulnerability is HIGH because this can impact ALL users. Furthermore, the longer a user keeps PTs with the protocol, the higher chance that the user can get the loss.

Proof of Concept

The following test code can be added to ContractPrincipalToken1. Only two user cases are shown here for simplicity, but it applies to any number of users. The code proves that any de-pegging event can cause the loss for users even if the market recovers, and the loss of users means the gain of the protocol. The brief flow for the code is as follows:

  1. ADDR_1 deposits assets to the PT contract.
  2. ADDR_2 deposits assets when ibtRate has decreased.
  3. The market has recovered, and ADDR_1 redeems assets when ibtRate has recovered.
    • ADDR_1 only receives a reduced amount of assets because of step 2.
    • The loss of ADDR_1 reflected in the gain of the protocol.
    function testDepegCauseUserLoss() public {
        // Amount of asset to deposit
        uint256 amount = 1e18; 

        // Equip ADDR_1/2 with assets
        underlying.mint(MOCK_ADDR_1, amount);
        underlying.mint(MOCK_ADDR_2, amount);

        // 1. MOCK_ADDR_1 deposits to PT
        vm.startPrank(MOCK_ADDR_1);
        underlying.approve(address(principalToken), amount);
        principalToken.deposit(amount, MOCK_ADDR_1);
        vm.stopPrank();

        // Check (redeemable) asset balance
        uint256 ptBalanceOfADDR_1 = principalToken.balanceOf(MOCK_ADDR_1);
        uint256 assetBalanceOfADDR_1 = principalToken.convertToUnderlying(ptBalanceOfADDR_1);

        uint256 ibtBalanceOfPT = IERC4626(ibt).balanceOf(address(principalToken));
        uint256 assetBalanceOfPT =  IERC4626(ibt).convertToAssets(ibtBalanceOfPT);

        assertEq(assetBalanceOfADDR_1, amount);
        assertEq(assetBalanceOfPT, amount);

        // 2. MOCK_ADDR_2 deposits when ibtRate decrease by 50% 
        _increaseRate(-50);
        vm.startPrank(MOCK_ADDR_2);
        underlying.approve(address(principalToken), amount);
        principalToken.deposit(amount, MOCK_ADDR_2);
        vm.stopPrank();

        // Check (redeemable) asset balance
        ptBalanceOfADDR_1 = principalToken.balanceOf(MOCK_ADDR_1);
        assetBalanceOfADDR_1 = principalToken.convertToUnderlying(ptBalanceOfADDR_1);

        ibtBalanceOfPT = IERC4626(ibt).balanceOf(address(principalToken));
        assetBalanceOfPT =  IERC4626(ibt).convertToAssets(ibtBalanceOfPT);

        uint256 ptBalanceOfADDR_2 = principalToken.balanceOf(MOCK_ADDR_2);
        uint256 assetBalanceOfADDR_2 = principalToken.convertToUnderlying(ptBalanceOfADDR_2);

        assertEq(assetBalanceOfADDR_1, amount/2);
        assertEq(assetBalanceOfADDR_2, amount);
        assertEq(assetBalanceOfPT, assetBalanceOfADDR_1 + assetBalanceOfADDR_2);

        // 3. Market recovered from depeg, and MOCK_ADD_1 redeems assets at maturity
        vm.warp(block.timestamp + principalToken.maturity());
        _increaseRate(100);

        // Check (redeemable) asset balance
        ptBalanceOfADDR_1 = principalToken.balanceOf(MOCK_ADDR_1);
        vm.prank(MOCK_ADDR_1);
        assetBalanceOfADDR_1 = principalToken.redeem(ptBalanceOfADDR_1, MOCK_ADDR_1, MOCK_ADDR_1);

        ibtBalanceOfPT = IERC4626(ibt).balanceOf(address(principalToken));
        assetBalanceOfPT =  IERC4626(ibt).convertToAssets(ibtBalanceOfPT);

        ptBalanceOfADDR_2 = principalToken.balanceOf(MOCK_ADDR_2);
        assetBalanceOfADDR_2 = principalToken.convertToUnderlying(ptBalanceOfADDR_2);

        assertEq(assetBalanceOfADDR_1, amount/2); // loss for ADDR_1
        assertEq(assetBalanceOfADDR_2, amount);
        assertEq(assetBalanceOfPT, amount/2 + amount*2); // "amount/2" is from ADDR_1's loss
    }

Tools Used

Foundry

Recommended Mitigation Steps

Currently, the protocol only penalizes PT holders if ibtRate decreases but does not compensate when the market recovers. There is a need to implement a mechanism to reflect the increased yield (at least for the market recovery) scenarios for PT holders. For example, the protocol may record a ptRate (as deposit ptRate) when a user deposits and consider increasing ptRate up to the deposit ptRate when the market recovers.

Assessed type

Other

c4-pre-sort commented 8 months ago

gzeon-c4 marked the issue as duplicate of #305

c4-pre-sort commented 8 months ago

gzeon-c4 marked the issue as sufficient quality report

c4-judge commented 8 months ago

JustDravee marked the issue as unsatisfactory: Invalid