Rounding in favour of the user in the _convertIBTsToShares() function

[c4-bot-2]

Lines of code

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

Vulnerability details

Impact

Rounding in favour of the user in the _convertIBTsToShares() function may lead to minting more shares than it was intended by the protocol. Minted amount of shares may exceed the amount that was supposed to be minted in case of rounding down in favour of the protocol, thus leading to loss of funds for the protocol when redeeming/withdrawing.

Proof of Concept

First of all, let's take a look at two invariants stated for this protocol:

1. Decimals imprecisions should always benefit the protocol and no user should be able to extract extra value;
2. previewDeposit ≤ deposit : the preview of shares minted upon depositing should be less than or equal to the actual shares minted.

Let's start from the second invariant and see its realisation in the code. This invariant is secured by the difference between the _convertIBTsToShares() and the _convertIBTsToSharesPreview() functions. The first difference is during the call to _getPTandIBTRates(), where _convertIBTsToShares() passes false, and the preview function passes true:

function _convertIBTsToShares(
        uint256 _ibts,
        bool _roundUp
    ) internal view returns (uint256 shares) {
        (uint256 _ptRate, uint256 _ibtRate) = _getPTandIBTRates(false);

   function _convertIBTsToSharesPreview(uint256 ibts) internal view returns (uint256 shares) {
        (uint256 _ptRate, uint256 _ibtRate) = _getPTandIBTRates(true);

In the code commentaries there is important note, stating that to round up the shares, the PT rate must round down. This statement is then once again found in the _getPTandIBTRates() function: roundUpPTRate true if the PTRate result from mulDiv computation in case of negative rate should be rounded up. Thus, in order to roundUp the shares, it is required to roundDown the _ptRate. By setting false for the _convertIBTsToShares() and true for _convertIBTsToSharesPreview() we definitely secure, that the amount of shares for preview will likely be less than the amount for the actual deposit() function. However, by securing this in the outlined way, we are actually giving a field for rounding not in favour of the protocol, but in favour of the user, as it is outlined in the natspec comments and as is it showed in the code itself, since the amount of shares is calculated in the following way: shares = ibts * ibtRate / ptRate. The higher the ptRate, the lower the amount of shares will be, the lower the ptRate, the higher the amount of shares will be. So, by passing the false bool-parameter to the _convertIBTsToShares, which function is actually used in the deposit flow, we are allowing for rounding in favour of the user, since the amount of shares will be rounded up, even though the previewDeposit ≤ deposit invariant is secured.

Details

We realise that there is also another difference in the implementations of these funcctions during the final calculations of shares:

function _convertIBTsToSharesPreview(uint256 ibts) internal view returns (uint256 shares) {
....
shares = ibts.mulDiv(_ibtRate, _ptRate);
}

function _convertIBTsToShares(
        uint256 _ibts,
        bool _roundUp
    ) internal view returns (uint256 shares) {
...
        shares = _ibts.mulDiv(
            _ibtRate,
            _ptRate,
            _roundUp ? Math.Rounding.Ceil : Math.Rounding.Floor
        );
    }

Rounding is kind of secured for the _convertIBTsToShares() function, but still it is important to make sure that in no place of the protocol there is a possibility to allow rounding directions to be in favour of the user.

Tools Used

Manual Review

Recommended Mitigation Steps

Firstly, change the false parameter to true in the _convertIBTsToShares() function during the call to the _getPTandIBTRates() function.

function _convertIBTsToShares(
        uint256 _ibts,
        bool _roundUp
    ) internal view returns (uint256 shares) {
        (uint256 _ptRate, uint256 _ibtRate) = _getPTandIBTRates(true);

This mitigation will not actually break the previewDeposit ≤ deposit invariant, since the result of the previewDeposit() and deposit() functions should then be always the same.

Assessed type

Other

[c4-pre-sort]

gzeon-c4 marked the issue as duplicate of #201

[c4-pre-sort]

gzeon-c4 marked the issue as sufficient quality report

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid