Improper Implementation of Slippage Protection Mechanism

[c4-bot-4]

Lines of code

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

Vulnerability details

Impact

Users could end up with fewer shares than they anticipated based on the assets they provided due to the incorrect implementation of slippage protection. Instead of ensuring a minimum number of shares received, the current implementation limits the maximum shares which does not align with the typical use case of slippage protection

Proof of Concept

Both withdraw and withdrawIBT functions in the PrincipalToken contract utilize a maxShares parameter intended for slippage protection. However, the naming and implementation logic of this parameter suggests a focus on limiting the maximum number of shares to be received on given assets.

 if (shares > maxShares) {
            revert ERC5143SlippageProtectionFailed();
        }

In financial transactions, the primary concern typically revolves around ensuring users do not receive too few shares rather than too many which is not the case here.

The slippage protection should guard users against unfavorable exchange rates that result in receiving an insufficient number of shares for their assets or IBTs.

Tools Used

Manual Review

Recommended Mitigation Steps

Rename the maxShares parameter to minShares to accurately reflect its purpose in slippage protection and adjust the conditional checks to ensure the number of calculated shares is not less than minShares

Assessed type

Other

[c4-pre-sort]

gzeon-c4 marked the issue as duplicate of #246

[c4-pre-sort]

gzeon-c4 marked the issue as insufficient quality report

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid