The convertToPrincipal and convertToUnderlying functions can potentially allow manipulation of rates after expiry.

[c4-bot-5]

Lines of code

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

Vulnerability details

Impact

After expiry, users can manipulate rates to get more or less principal tokens for their underlying assets or vice versa when converting between the two. This could benefit certain users unfairly. It could enable certain attacks like flash loan arbitrage after expiry by artificially modifying rates. An attacker could take a flash loan, convert it to principal tokens using a manipulated low rate, redeem principal tokens using a manipulated high rate, and profit from it. It goes against the concept of storing rates at expiry to finalize conversion rates. Allowing manipulation afterwards undermines this. Severity: Medium Overall, I would consider the severity of this issue as High since it opens up flash loan attack vectors.

Proof of Concept

The convertToPrincipal and convertToUnderlying functions can potentially allow manipulation of rates after expiry. The key issue is that these functions call previewDeposit and previewRedeem on the IBT even after expiry. Specifically:

   /** @dev See {IPrincipalToken-convertToPrincipal}. */
   function convertToPrincipal(uint256 underlyingAmount) external view 
   override returns (uint256) {
       return 
   _convertIBTsToShares(IERC4626(ibt).previewDeposit(underlyingAmount), 
   false); 
   }

   /** @dev See {IPrincipalToken-convertToUnderlying}. */  
   function convertToUnderlying(uint256 principalAmount) public view override 
   returns (uint256) {
       return 
   IERC4626(ibt).previewRedeem(_convertSharesToIBTs(principalAmount, false));
   }

The previewDeposit and previewRedeem functions on the IBT use the current IBT rate. However, after expiry the IBT rate could change while the PT rate is fixed based on storeRatesAtExpiry(). This means by manipulating the IBT rate through deposits/redeems after expiry, an attacker could manipulate the convertToPrincipal and convertToUnderlying view functions to return incorrect results. For example, an attacker could:

1. Wait until after expiry when rates are stored
2. Deposit assets into the IBT to artificially increase IBT rate
3. Call convertToPrincipal - this will use the new higher IBT rate to convert assets to higher PT shares
4. Withdraw from the IBT to bring rate back down
5. Call convertToUnderlying - this will use the old lower PT rate to convert PT shares to more assets

Tools Used

Manual

Recommended Mitigation Steps

the convertToPrincipal and convertToUnderlying functions should use the stored rates after expiry rather than calling the IBT contract. For example:

   function convertToPrincipal(uint256 underlyingAmount) external view 
   override returns (uint256) {
       if (block.timestamp >= expiry) {
           return _convertIBTsToShares(underlyingAmount, false); 
       } else {
           return 
   _convertIBTsToShares(IERC4626(ibt).previewDeposit(underlyingAmount), 
   false);
       }
    }

This will prevent manipulation of the rate oracles after expiry.

Assessed type

Other

[c4-pre-sort]

gzeon-c4 marked the issue as primary issue

[c4-pre-sort]

gzeon-c4 marked the issue as sufficient quality report

[c4-sponsor]

yanisepfl (sponsor) disputed

[yanisepfl]

After expiry, the users get assigned an IBT value assigned to them that they can get through withdraw/redeem and claimYield. This value is found using the rates stored at expiry. That being said, after expiry, the IBT vault can still have a volatile rate, which is OK. However, in convertToPrincipal and convertToUnderlying we also need to take this into account. And this is exactly what is being done:

For example, for convertToPrincipal:

function convertToPrincipal(uint256 underlyingAmount) external view override returns (uint256) {
        return _convertIBTsToShares(IERC4626(ibt).previewDeposit(underlyingAmount), false);
    }

where we check what the current asset value in IBT would have been worth in shares using the stored rate after expiry! Indeed, the _convertIBTsToShares uses the stored rates after expiry.

The explanation is very similar for convertToUnderlying.

Therefore, the issue is wrong and we dispute it.

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid