Lack of `minAsset` check in `claimYield()` and `claimFees()`.

[c4-bot-10]

Lines of code

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

Vulnerability details

Bug description

claimFees() function allows the fee collector set by the protocol to claim fees in IBT. claimFees()

function claimFees() external override returns (uint256 assets) {
        if (msg.sender != IRegistry(registry).getFeeCollector()) {
            revert UnauthorizedCaller();
        }
        uint256 ibts = unclaimedFeesInIBT;
        unclaimedFeesInIBT = 0;
        assets = IERC4626(ibt).redeem(ibts, msg.sender, address(this));
        emit FeeClaimed(msg.sender, ibts, assets);
    }

claimYield() allows users to claim their yield in IBT. claimYield()

function claimYield(address _receiver) public override returns (uint256 yieldInAsset) {
        uint256 yieldInIBT = _claimYield();
        if (yieldInIBT != 0) {
            yieldInAsset = IERC4626(ibt).redeem(yieldInIBT, _receiver, address(this));
        }
    }

As can be seen, both these functions redeem ibts from the ERC4626 vault, but they lack minAssets check. To protect users or feeCollector from receiving less assets for their amount of ibts, minAssets parameter should be used.

Impact

The entire amount of ibts can be lost.

Recommended Mitigation

claimYield()

- function claimYield(address _receiver) public override returns (uint256 yieldInAsset) {
+ function claimYield(address _receiver, uint256 minAssets) public override returns (uint256 yieldInAsset) {
        uint256 yieldInIBT = _claimYield();
        if (yieldInIBT != 0) {
            yieldInAsset = IERC4626(ibt).redeem(yieldInIBT, _receiver, address(this));
+           require(yieldInAsset >= minAssets);
        }
    }

claimFees()

- function claimFees() external override returns (uint256 assets) {
+ function claimFees(uint256 minAssets) external override returns (uint256 assets) {
        if (msg.sender != IRegistry(registry).getFeeCollector()) {
            revert UnauthorizedCaller();
        }
        uint256 ibts = unclaimedFeesInIBT;
        unclaimedFeesInIBT = 0;
        assets = IERC4626(ibt).redeem(ibts, msg.sender, address(this));
+       require(assets >= minAssets);
        emit FeeClaimed(msg.sender, ibts, assets);
    }

Assessed type

Other

[c4-pre-sort]

gzeon-c4 marked the issue as insufficient quality report

[gzeon-c4]

unlikely for "claim slippage" to exists and the protocol should record claimed amount lack poc to show exploit

[c4-pre-sort]

gzeon-c4 marked the issue as primary issue

[c4-judge]

JustDravee marked the issue as unsatisfactory: Insufficient proof

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid

[kazantseff]

Hey @JustDravee, The PT contract is built on top of the ERC4626 vault and the whole idea of it, is that rates are subject to volatility, so the claim slippage will exist. redeem function of the PT contract has slippage protection and it does exactly the same action as two of the function described here. Could you please take another look?

[c4-sponsor]

yanisepfl (sponsor) disputed

[c4-sponsor]

yanisepfl marked the issue as disagree with severity

[yanisepfl]

Hello @kazantseff and @JustDravee,

While the issue reported is a good catch that we will tackle, it only points out an inconsistency in our code. The standards that we follow do not necessitate to have slippage protection, and we consider it as a good-to-have feature.

Therefore, we rather consider it as a low severity issue.

Thanks for the report!

[c4-judge]

JustDravee removed the grade

[c4-judge]

JustDravee changed the severity to QA (Quality Assurance)

[c4-judge]

JustDravee marked the issue as grade-b

[c4-judge]

JustDravee marked the issue as grade-a