`withdraw()` and `withdrawIBT()` rounds in incorrect direction

[c4-bot-1]

Lines of code

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

Vulnerability details

Impact

The current implementation of withdraw() and withdrawIBT() rounds in incorrect direction. Rounding in the incorrect direction may cause loss to the protocol.

Proof of Concept

According to [EIP-5095](https://github.com/ethereum/ercs/blob/master/ERCS/erc-5095.md#reference-implementation, we can see the example of withdraw() implementation:

principalAmount = _previewWithdraw(underlyingAmount); // No need to check for rounding error, previewWithdraw rounds up.

As demonstrated in the code above - it should round up: No need to check for rounding error, previewWithdraw rounds up

Since previewWithdraw() rounds up - similarly - withdraw() should also round up. Let's take a look at its implementation in the PrincipalToken.sol:

File: PrincipalToken.sol:

(uint256 _ptRate, uint256 _ibtRate) = _getPTandIBTRates(false);

Function _getPTandIBTRates()'s parameter defines if the result should be rounded up (parameter should be true) or down (parameter should be false). The current implementation rounds down (parameter is false) for the withdraw function - which is incorrect. While we deposit assets, the rounding should be down (in favor on the protocol) - withdrawing should also work in favor of the protocol - thus the rounding should be up. However, the current implementation rounds down, instead of up.

It's important to notice, that in some cases, ERC-5095 should be cross-compatible with ERC-4626 - since it shares the same functionality. When we take a look at ERC-4626 implementation (OZ's implementation) - we can confirm, that withdrawing requires rounding up:

source: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/105fa4e1b0832a6a40cb7ba6e545bb844f02a711/contracts/token/ERC20/extensions/ERC4626.sol#L161-L163

    function previewWithdraw(uint256 assets) public view virtual returns (uint256) {
        return _convertToShares(assets, Math.Rounding.Ceil);
    }

However, the current implementation rounds down _getPTandIBTRates() is set to false instead of true.

The same issue occurs for withdrawIBT() - it also rounds down, instead of rounding up.

Tools Used

Manual code review

Recommended Mitigation Steps

Change line (uint256 _ptRate, uint256 _ibtRate) = _getPTandIBTRates(false); to (uint256 _ptRate, uint256 _ibtRate) = _getPTandIBTRates(true);

Assessed type

Math

[c4-pre-sort]

gzeon-c4 marked the issue as sufficient quality report

[c4-pre-sort]

gzeon-c4 marked the issue as primary issue

[c4-pre-sort]

gzeon-c4 marked the issue as insufficient quality report

[gzeon-c4]

rate round down should mean share rounded up

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid