c4-bot-7 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/main/src/tokens/PrincipalToken.sol#L906-L913

Vulnerability details

Impact

Vault which IBT rate often alternates between increasing and decreasing will quickly push the principal token PT rate to a low number.

Proof of Concept

Let's look at how PT and IBT rates are updated in the PrincipalToken.sol

        uint256 currentPTRate = currentIBTRate < ibtRate
            ? ptRate.mulDiv(
                currentIBTRate,
                ibtRate,
                roundUpPTRate ? Math.Rounding.Ceil : Math.Rounding.Floor
            )
            : ptRate;
        return (currentPTRate, currentIBTRate);
    }

If the previous IBT rate is greater than the current one, we calculate the new PT rate as PTnew = PTold * IBTnew / IBTold, the PT rate never increases, and can only decrease.

Let's say we have a vault which IBT rate = 1 has decreased to 0.9, as a result the PT rate will be 1 0.9 / 1 = 0.9, after some time the IBT rate increased to 1 and then decreased to 0.9 again, thus decreasing PT rate even more 0.9 0.9 / 1 = 0.81. It's easy to notice that if these small fluctuations continue, the PT rate will soon go down to a small number.

Coded POC for PrincipalToken.t.sol

    function testPTRateDegrade() public {
        uint256 amountToDeposit = 100e18;
        underlying.approve(address(principalToken), amountToDeposit);

        // initial deposit
        principalToken.deposit(amountToDeposit, address(this));

        for(uint256 i=0; i<20; i++){
            // IBT rate goes down by 5%
            _increaseRate(-5);
            // poke update rate
            principalToken.updateYield(address(this));
            // IBT rate goes up by 5%
            _increaseRate(5);
            principalToken.updateYield(address(this));
        }
        ibtRate = principalToken.getIBTRate();
        ptRate = principalToken.getPTRate();
        console.log("IBT PT: ", ibtRate, ptRate);
    }

It will take twenty 5 % fluctuations to decrease the PT rate to 0.34

Tools Used

Foundry

Recommended Mitigation Steps

Perhaps we need to decrease the PT rate only if the IBT rate updated it's minimum

    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;
>>      if (currentIBTRate < ibtRateMin) {
            currentPTRate = ptRate.mulDiv(
                currentIBTRate,
                ibtRate,
                roundUpPTRate ? Math.Rounding.Ceil : Math.Rounding.Floor
            )
>>          ibtRateMin = ibtRate;
        } else {
            currentPTRate = ptRate;
        }
        return (currentPTRate, currentIBTRate);
    }

Assessed type

ERC4626

c4-pre-sort commented 6 months ago

gzeon-c4 marked the issue as duplicate of #53

c4-pre-sort commented 6 months ago

gzeon-c4 marked the issue as sufficient quality report

c4-judge commented 6 months ago

JustDravee marked the issue as unsatisfactory: Invalid

k4zanmalay commented 6 months ago

@JustDravee I believe this report was incorrectly duped with #53. The idea of this finding is that in vaults where IBT rate is volatile, multiple small fluctuations (e.g. 5%) in the said rate will cause losses to accumulate on the PT rate. For example, if the IBT rate alternates between increasing and decreasing in 1-0.95 range, the PT rate will decrease to 0.35, as it would be if the IBT rate decreased to the same value.

c4-sponsor commented 6 months ago

yanisepfl (sponsor) disputed

yanisepfl commented 6 months ago

Hello @k4zanmalay @JustDravee,

I believe this report was incorrectly duped with https://github.com/code-423n4/2024-02-spectra-findings/issues/53

Correct those are not duplicates.

in vaults where IBT rate is volatile, multiple small fluctuations (e.g. 5%) in the said rate will cause losses to accumulate on the PT rate.

You are right except this is how our protocol was designed. In such a scenario, the PT worth in asset in our protocol would indeed decrease towards 0, and the YT holders will see their yield increase. Also, for such IBTs, markets should price tokens accordingly, i.e. PT traded at smaller costs than "usual" and YT traded at higher costs than "usual".

I hope that clarifies things for you.

Therefore, we dispute this issue.