The ratesAtExpiryStored flag is not protected by access control. Anyone can call storeRatesAtExpiry() after expiry

[c4-bot-9]

Lines of code

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

Vulnerability details

Impact

Anyone can lock in potentially arbitrary rates for redeeming principal tokens after expiry by calling storeRatesAtExpiry(). This could allow attackers to manipulate redemptions to their advantage. The severity is high because it impacts a critical part of the contract's functionality for redeeming tokens after expiry. Allowing anyone to determine the conversion rates means the rates could potentially be set to completely arbitrary values, enabling theft of funds or making redemptions impossible if rates are set to zero.

Proof of Concept

The ratesAtExpiryStored flag is not protected by access control. This means that anyone can call the storeRatesAtExpiry() function after expiry. Here is how it works: The storeRatesAtExpiry() function is marked with the afterExpiry modifier, which checks that the block timestamp is greater than or equal to the expiry time:

   modifier afterExpiry() virtual {
       if (block.timestamp < expiry) {
           revert PTNotExpired();
      }
       _;
   }

This means that anyone can call the storeRatesAtExpiry() function after the expiry time. Inside the function, it simply sets the ratesAtExpiryStored flag to true:

   function storeRatesAtExpiry() public override afterExpiry {
       if (ratesAtExpiryStored) {
           revert RatesAtExpiryAlreadyStored(); 
       }
       ratesAtExpiryStored = true;

       // update rates

       emit RatesStoredAtExpiry(...);
  }

There is no access control check here to restrict who can call this. The impact is that anyone can trigger the rate storage after expiry. This could be used by an attacker to manipulate the rates stored if the actual market rates have moved significantly. For example, an attacker could:

1. Wait until expiry
2. Call storeRatesAtExpiry() with the old rates if rates have dropped a lot
3. Redeem PT for less IBT than they should get

Tools Used

Manual

Recommended Mitigation Steps

The storeRatesAtExpiry() function should be restricted to an authorized role, for example:

   function storeRatesAtExpiry() public override afterExpiry restricted {
     // add restricted modifier
   }

This would prevent unauthorized actors from manipulating the stored rates.

Assessed type

Other

[c4-pre-sort]

gzeon-c4 marked the issue as duplicate of #282

[c4-pre-sort]

gzeon-c4 marked the issue as insufficient quality report

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid