`ptRate` can be set to 0, which will freeze funds held by the PrincipalToken contract.

[c4-bot-10]

Lines of code

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

Vulnerability details

Bug description

PrincipalToken contract allows user to tokenize their yield by depositing underlying assets or shares of the underlying ERC4626 vault. The rate which is used to issue new principal tokens is determined by the ibtRate and ptRate. ibtRate is the rate at which one ibtUnit can be redeemed for the underlying asset, it can increase or decrease and it influences the ptRate. ibtRate is updated upon user interaction with the protocol, while ptRate is only updated after an accounted negative rate change on the ibtRate.

_getCurrentPTandIBTRates()

 function _getCurrentPTandIBTRates(bool roundUpPTRate) internal view returns (uint256, uint256) {
        uint256 currentIBTRate = IERC4626(ibt).previewRedeem(ibtUnit).toRay(_assetDecimals);
        if (IERC4626(ibt).totalAssets() == 0 && IERC4626(ibt).totalSupply() != 0) {
            currentIBTRate = 0;
        }
        uint256 currentPTRate = currentIBTRate < ibtRate
            ? ptRate.mulDiv(
                currentIBTRate,
                ibtRate,
                roundUpPTRate ? Math.Rounding.Ceil : Math.Rounding.Floor
            )
            : ptRate;
        return (currentPTRate, currentIBTRate);
    }

The function above shows how these rates are calculated and set. The scenario where the currentIBTRate is set to 0 happens in a situation where either IBTs have been hacked and stolen all their deposits or a malicious IBT. I will not review the scenario of malicious IBT as it is out of scope, but I will focus on the first case. First of all, let's discuss why and how setting currentIBTRate to 0 can lead to problems. As can be seen from the function, when currentIBTRate is 0, the calculations of currentPTRate will result in:

ptRate * 0 / ibtRate = 0

This is problematic, because unlike ibtRate, ptRate can never increase. ptRate being 0 leads to the inability for users to ever withdraw their deposited IBTs. This happens due to redeem() function calling _convertSharesToIBTs(), where calculations of ibt to redeem will result in 0, and due to withdraw() calling _withdrawShares(), which will revert when ptRate is 0.

_convertSharesToIBTs()

function _convertSharesToIBTs(
        uint256 _shares,
        bool _roundUp
    ) internal view returns (uint256 ibts) {
        (uint256 _ptRate, uint256 _ibtRate) = _getPTandIBTRates(false);
        if (_ibtRate == 0) {
            revert RateError();
        }
        ibts = _shares.mulDiv(
            _ptRate,
            _ibtRate,
            _roundUp ? Math.Rounding.Ceil : Math.Rounding.Floor
        );
    }

_withdrawShares()

function _withdrawShares(
        uint256 _ibts,
        address _receiver,
        address _owner,
        uint256 _ptRate,
        uint256 _ibtRate
    ) internal returns (uint256 shares) {
        if (_ptRate == 0) {
            revert RateError();
        }
        // convert ibts to shares using provided rates
        shares = _ibts.mulDiv(_ibtRate, _ptRate, Math.Rounding.Ceil);
        // burn owner's shares (YT and PT)
        if (block.timestamp < expiry) {
            IYieldToken(yt).burnWithoutUpdate(_owner, shares);
        }
        _burn(_owner, shares);
        emit Redeem(_owner, _receiver, shares);
    }

Now let's discuss the mentioned scenario under which this situation can occur. If a non-malicious IBT has lost all of its funds, there is still a non-zero chance it will be able to recover. But because ptRate is set to 0 and can NEVER increase, all of the holders of that PrincipalToken won't ever have a chance to recover their funds, they will be forever stuck on the balance of the PT contract.

Impact

Because ptRate can never increase, after it is set to 0, all of the user funds will be stuck on PrincipalToken contract without a way to rescue them.

Proof of Concept

Please add the following function to PrincipalToken3.t.sol and run the test with forge test --match-test 'testUnableToWithdrawWhenRateIsZero'.

function testUnableToWithdrawWhenRateIsZero() public {
        address user1 = makeAddr("user1");
        address user2 = makeAddr("user2");
        vm.prank(user1);
        underlying.approve(address(principalToken), type(uint256).max);
        underlying.mint(user1, 100e18);

        // User deposits 100 underlying
        vm.prank(user1);
        principalToken.deposit(100e18, user1);

        // IBT lost all of its assets
        underlying.burn(address(ibt), underlying.balanceOf(address(ibt)));

        // ptRate is set to 0
        principalToken.updateYield(user2);
        assertEq(principalToken.getPTRate(),0);
        // IBT is able to recover its funds
        underlying.mint(address(ibt), 1000e18);

        // User tries to withdraw his ibts
        vm.startPrank(user1);
        // Redeeming does not work, as amount of ibt to redeem results in 0 due to ptRate being 0
        principalToken.redeem(100e18, user1, user1);
        assertEq(underlying.balanceOf(user1), 0);
        // Withdraw reverts with rateError due to ptRate being 0
        vm.expectRevert();
        principalToken.withdraw(100e18, user1, user1);
        vm.stopPrank();
    }

Recommended Mitigation

Ideally ptRate should never be set to 0 and it's impossible under normal conditions. Instead of setting it to 0 in such a situation, pause the contract, until IBT is able to resolve the issue. _getCurrentPTandIBTRates()

function _getCurrentPTandIBTRates(bool roundUpPTRate) internal view returns (uint256, uint256) {
        uint256 currentIBTRate = IERC4626(ibt).previewRedeem(ibtUnit).toRay(_assetDecimals);
        if (IERC4626(ibt).totalAssets() == 0 && IERC4626(ibt).totalSupply() != 0) {
-            currentIBTRate = 0;
+            _pause();
+            return;
        }
        uint256 currentPTRate = currentIBTRate < ibtRate
            ? ptRate.mulDiv(
                currentIBTRate,
                ibtRate,
                roundUpPTRate ? Math.Rounding.Ceil : Math.Rounding.Floor
            )
            : ptRate;
        return (currentPTRate, currentIBTRate);
    }

Assessed type

Other

[c4-pre-sort]

gzeon-c4 marked the issue as sufficient quality report

[c4-pre-sort]

gzeon-c4 marked the issue as primary issue

[gzeon-c4]

I think this is intended, when ptrate decreased any further increase is treated as yield to yt

[c4-sponsor]

yanisepfl (sponsor) disputed

[yanisepfl]

I think this is intended, when ptrate decreased any further increase is treated as yield to yt

What is stated there is absolutely correct, except if rates have gone to 0, in which case nothing can be done anymore. Indeed, in the catastrophic situation where the IBT rate goes to 0 (non malicious IBT hacked, malicious IBT etc...), depositing/redeeming in and out of the IBT does not make sense anymore (e.g. if the IBT rate is 0, we could expect a user to deposit very small amounts and obtain huge amounts of IBTs). Therefore, what happens after such a scenario is out-of-scope. Also, if it was a non-malicious protocol, and that it recovered funds, it would be up to the hacked protocol to carefully investigate how to redistribute recovered funds to users (which should be done without considering post attack IBT/PT/YT holdings which might be completely off when it comes to who should recover what).

The issue is thus disputed.

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid