Incorrect rounding in `_convertIBTsToShares()`/`_convertSharesToIBTs()`, leading to minting more shares for the user than intended

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/main/src/tokens/PrincipalToken.sol#L680-L693 https://github.com/code-423n4/2024-02-spectra/blob/main/src/tokens/PrincipalToken.sol#L659-L672

Vulnerability details

Impact

The rounding of _convertIBTsToShares() and _convertSharesToIBTs() function is incorrect, and does not always benefit the protocol. This could result in the minting of more shares for the user than intended, disadvantaging the protocol.

Bug Description

There are two rounding issues in the code:

1. _convertIBTsToShares(), the more important issue, which may lead to user minting more PT/YTs than intended.
2. _convertSharesToIBTs(), the less important issue, does not cause any main issues at the moment, but is still coded incorrectly.

1. _convertIBTsToShares()

The _convertIBTsToShares function is designed to convert IBTs into PT/YT shares, the _roundUp parameter dictates the rounding direction. In scenarios where _roundUp == false, indicating a preference for rounding down. The current implementation incorrectly passes false to the _getPTandIBTRates function. This results in rounding down of the _ptRate, which, upon division to calculate the share amount, leads to rounding up the final share quantity.

This means even if the function is called with _roundUp == false, it may still round up. This discrepancy can impact the _depositIBT function, leading it to mint an amount of shares for the user that is slightly more than what is strictly proportional, favoring the user over the protocol. This could allow users to gain additional value, which is not the intended behavior.

    function _convertIBTsToShares(
        uint256 _ibts,
        bool _roundUp
    ) internal view returns (uint256 shares) {
>       (uint256 _ptRate, uint256 _ibtRate) = _getPTandIBTRates(false);
        if (_ptRate == 0) {
            revert RateError();
        }
        shares = _ibts.mulDiv(
            _ibtRate,
            _ptRate,
            _roundUp ? Math.Rounding.Ceil : Math.Rounding.Floor
        );
    }
    ...
    function _getCurrentPTandIBTRates(bool roundUpPTRate) internal view returns (uint256, uint256) {
        uint256 currentIBTRate = IERC4626(ibt).previewRedeem(ibtUnit).toRay(_assetDecimals);
        if (IERC4626(ibt).totalAssets() == 0) { // @auditnote: Q: Why check totalSupply != 0?
            currentIBTRate = 0;
        }
        uint256 currentPTRate = currentIBTRate < ibtRate
            ? ptRate.mulDiv(
                currentIBTRate,
                ibtRate,
                roundUpPTRate ? Math.Rounding.Ceil : Math.Rounding.Floor
            )
            : ptRate;
        return (currentPTRate, currentIBTRate);
    }
    function _getPTandIBTRates(bool roundUpPTRate) internal view returns (uint256, uint256) {
        if (ratesAtExpiryStored) {
            return (ptRate, ibtRate);
        } else {
            return _getCurrentPTandIBTRates(roundUpPTRate);
        }
    }

    function _depositIBT(
        uint256 _ibts,
        address _ptReceiver,
        address _ytReceiver
    ) internal notExpired nonReentrant whenNotPaused returns (uint256 shares) {
        updateYield(_ytReceiver);
        uint256 tokenizationFee = PrincipalTokenUtil._computeTokenizationFee(
            _ibts,
            address(this),
            registry
        );
        _updateFees(tokenizationFee);
>       shares = _convertIBTsToShares(_ibts - tokenizationFee, false);
        if (shares == 0) {
            revert RateError();
        }
        _mint(_ptReceiver, shares);
        emit Mint(msg.sender, _ptReceiver, shares);
        IYieldToken(yt).mint(_ytReceiver, shares);
    }

2. _convertSharesToIBTs()

The root cause is the same as above: the _getPTandIBTRates function always passes in false. However, since all current calls to _convertSharesToIBTs passes in _roundUp == false, the impact of this error is not severe. Instead, it presents itself more as a coding oversight than a critical flaw.

    function _convertSharesToIBTs(
        uint256 _shares,
        bool _roundUp
    ) internal view returns (uint256 ibts) {
>       (uint256 _ptRate, uint256 _ibtRate) = _getPTandIBTRates(false);
        if (_ibtRate == 0) {
            revert RateError();
        }
        ibts = _shares.mulDiv(
            _ptRate,
            _ibtRate,
            _roundUp ? Math.Rounding.Ceil : Math.Rounding.Floor
        );
    }

Proof of Concept

If _ptRate rounds down, shares would round up. Following is the mathematic formula:

shares = _ibts * _ibtRate / _ptRate;
_ptRate = ptRate * currentIBTRate / ibtRate;

Tools Used

Manual review.

Recommended Mitigation Steps

1. Change false to !_roundUp for _convertIBTsToShares().
2. Change false to _roundUp for _convertSharesToIBTs().

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-sponsor]

yanisepfl (sponsor) disputed

[yanisepfl]

The _getPTandIBTRates allows for rounding up or down the PT rate obtained from the IBT rate change in the situation where the IBT rate has decreased, only for respecting the 5095 invariant that previewDeposit <= deposit. In any other usage of this method, it is best to round down the PT rate derived from the IBT rate decrease.

For instance, in the important issue mentioned by the auditor, rounding down the PT rate would indeed lead to a larger amount of shares (PTs/YTs) than if we had rounded up the PT rate. However, the auditor does not take into account the value of those PT. Indeed, having more of something that is worth less (more shares, PT rate rounded down) is not necessarily worst than having less of something that is worth more (less shares, PT rate rounded up). In particular, our protocol was designed so that the protocol is always favored.

Same principle goes for the "the less important issue" reported by the auditor.

Therefore, while the following statement:

If _ptRate rounds down, shares would round up.

is correct, the auditor did not take into account the PTs worth in assets, which matters here.

This issue is therefore incorrect and we dispute it.

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid