In line 253, `PrincipalToken::redeemForIBT` allows users to redeem without slippage protection

[c4-bot-3]

Lines of code

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

Vulnerability details

Impact

In PrincipalToken::redeemForIBT @ line 253 users are allowed to redeem ibts without slippage protection because the function is exposed public as shown below.

function redeemForIBT(
    uint256 shares,
    address receiver,
    address owner
) public override returns (uint256 ibts) { // @audit set as "public", should just be "internal"
    _beforeRedeem(shares, owner);
    ibts = _convertSharesToIBTs(shares, false);
    IERC20(ibt).safeTransfer(receiver, ibts);
    emit Redeem(owner, receiver, shares);
}

It was invoked on another function with the same name PrincipalToken::redeemForIBT @ line 265 but this time, it has slippage protection as shown below.

function redeemForIBT(
    uint256 shares,
    address receiver,
    address owner,
    uint256 minIbts
) external override returns (uint256 ibts) {
    ibts = redeemForIBT(shares, receiver, owner); // @audit redeemForIBT @ line 253 was invoked here
    if (ibts < minIbts) {
        revert ERC5143SlippageProtectionFailed();
    }
}

If the user calls PrincipalToken::redeemForIBT @ line 253, he/she is susceptible to inconceivable ibts loss. To redeem for ibts, the user must call PrincipalToken::redeemForIBT @ line 265 because it has the "slippage protection" and it should be explicitly and intentionally enforced.

Tools Used

Manual Review

Recommended Mitigation Steps

The quickest fix is to make PrincipalToken::redeemForIBT @ line 253 from public to internal as shown below.

function redeemForIBT(
    uint256 shares,
    address receiver,
    address owner
-- ) public override returns (uint256 ibts) {
++ ) internal override returns (uint256 ibts) {
    _beforeRedeem(shares, owner);
    ibts = _convertSharesToIBTs(shares, false);
    IERC20(ibt).safeTransfer(receiver, ibts);
    emit Redeem(owner, receiver, shares);
}

This will prevent the users from calling this function and instead use the "proper" redeemForIBT function which is PrincipalToken::redeemForIBT @ line 265 which has the slippage protection.

Assessed type

Other

[c4-pre-sort]

gzeon-c4 marked the issue as insufficient quality report

[gzeon-c4]

user error

[c4-pre-sort]

gzeon-c4 marked the issue as duplicate of #246

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid