Spectra - lsaudit's results

Lines of code

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

Vulnerability details

Impact

According to the provided documentation: Principal Token is ERC5095. This is the main invariant defined in the project's description:

File: README.md

This is the core contract of Spectra. The Principal Token is [EIP-5095](https://eips.ethereum.org/EIPS/eip-5095)

File: README.md

**Principal Token is ERC5095**

During the code-review process, some deviations from the EIP-5095 had been detected. Lack of compliance may cause unexpected behavior. Other protocols that integrate with contract may incorrectly assume that it's EIP-5095 compliant - especially that documentation states that it's ERC-5095. Any deviation from this standard will broke the composability and may lead to fund loss. While protocol's implements a contract and describes it as ERC-5095, it should fully conform to ERC-5095 standard.

According to EIP-5095, function maxWithdraw() MUST return 0 when withdrawals are entirely (or even temporarily) disabled. The current implementation does not follow this requirement. Moreover, according to EIP-5095, function maxWithdraw() MUST NOT revert. The current implementation does not follow this requirement.

The very similar issue (functions maxWithdraw and maxRedeem do not return zero - when withdrawal is paused) was evaluated as Medium during the previous Code4rena contest:

• https://github.com/code-423n4/2023-05-maia-findings/issues/585

The very similar issue (functions max* do not return 0 when contract is paused) was evaluated as High during the previous Code4rena contest:

• https://github.com/code-423n4/2022-09-y2k-finance-findings/issues/47

Moreover, during the previous C4 contests, lack of EIP compliance was usually evaluated as High/Medium

• (Lack of EIP-4626 compliance) https://github.com/code-423n4/2022-09-y2k-finance-findings/issues/47 - has been evaluated as High
• (Lack of EIP-4524 compliance) https://github.com/code-423n4/2022-10-holograph-findings/issues/440 - has been evaluated as Medium
• (Lack of EIP-721 compliance) https://github.com/code-423n4/2022-10-holograph-findings/issues/203 - has been evaluated as Medium

Taking into consideration the previous contests, I've decided to evaluate this issue as Medium.

Proof of Concept

PrincipalToken inherits from PausableUpgradeable and implements external pause()/unPause() functions - thus it can be paused.

According to the provided documentation, PrincipalToken.sol should be compliant with ERC5095:

File: README.md

EIP Compliance:
PrincipalToken.sol : Should comply with ERC5095 and ERC2612

According to EIP-5095, function maxWithdraw():

MUST factor in both global and user-specific limits, like if withdrawals are entirely disabled (even temporarily) it MUST return 0.

MUST NOT revert.

However, the current implementation of maxWithdraw does not obey above rules.

While protocol is paused, according to EIP-5095 - function maxWithdraw MUST return 0: if withdrawals are entirely disabled (even temporarily) it MUST return 0..

Moreover, function MUST NOT revert.

The current implementation implements whenNotPaused modifier. This implies, that when protocol is paused - function maxWithdraw won't return 0 - but it will revert. This violates two EIP-5095 rules (MUST NOT revert and if withdrawals are entirely disabled (even temporarily) it MUST return 0)

File: PrincipalToken.sol

    function maxWithdraw(address owner) public view override whenNotPaused returns (uint256) {
        return convertToUnderlying(_maxBurnable(owner));
    }

Tools Used

Manual code review

Recommended Mitigation Steps

When protocol is paused, maxWithdraw() should return 0, instead of reverting.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

According to the provided documentation: Principal Token is ERC5095. This is the main invariant defined in the project's description:

File: README.md

This is the core contract of Spectra. The Principal Token is [EIP-5095](https://eips.ethereum.org/EIPS/eip-5095)

File: README.md

**Principal Token is ERC5095**

During the code-review process, some deviations from the EIP-5095 had been detected. Lack of compliance may cause unexpected behavior. Other protocols that integrate with contract may incorrectly assume that it's EIP-5095 compliant - especially that documentation states that it's ERC-5095. Any deviation from this standard will broke the composability and may lead to fund loss. While protocol's implements a contract and describes it as ERC-5095, it should fully conform to ERC-5095 standard.

According to EIP-5095, function maxRedeem() MUST return 0 when withdrawals are entirely (or even temporarily) disabled. The current implementation does not follow this requirement.

The very similar issue (functions maxWithdraw and maxRedeem do not return - when withdrawal is paused) was evaluated as Medium during the previous Code4rena contest:

• https://github.com/code-423n4/2023-05-maia-findings/issues/585

The very similar issue (functions max* do not return 0 when contract is paused) was evaluated as High during the previous Code4rena contest:

• https://github.com/code-423n4/2022-09-y2k-finance-findings/issues/47

Moreover, during the previous C4 contests, lack of EIP compliance was usually evaluated as High/Medium

• (Lack of EIP-4626 compliance) https://github.com/code-423n4/2022-09-y2k-finance-findings/issues/47 - has been evaluated as High
• (Lack of EIP-4524 compliance) https://github.com/code-423n4/2022-10-holograph-findings/issues/440 - has been evaluated as Medium
• (Lack of EIP-721 compliance) https://github.com/code-423n4/2022-10-holograph-findings/issues/203 - has been evaluated as Medium

Taking into consideration the previous contests, I've decided to evaluate this issue as Medium.

Proof of Concept

PrincipalToken inherits from PausableUpgradeable and implements external pause()/unPause() functions - thus it can be paused.

According to EIP-5095, function maxRedeem():

MUST factor in both global and user-specific limits, like if redemption is entirely disabled (even temporarily) it MUST return 0.

However, please notice, that maxRedeem() implementation ignores the fact if protocol is paused or not.

File: PrincipalToken.sol

    function maxRedeem(address owner) public view override returns (uint256) {
        return _maxBurnable(owner);
    }

File: PrincilapToken.sol

    function _maxBurnable(address _user) internal view returns (uint256 maxBurnable) {
        if (block.timestamp >= expiry) {
            maxBurnable = balanceOf(_user);
        } else {
            uint256 ptBalance = balanceOf(_user);
            uint256 ytBalance = IYieldToken(yt).balanceOf(_user);
            maxBurnable = (ptBalance > ytBalance) ? ytBalance : ptBalance;
        }
    }

Function maxRedeem() calls _maxBurnable() - none of these functions takes care of the scenario when the contract is paused. Whenever the contract is paused, maxRedeem() will return the same value as it would return on the unpaused contract - even though, the EIP-5095 requires, that if redemption is entirely disabled (even temporarily) it MUST return 0.

This basically means, that maxRedeem() is not compliant with EIP-5095. On the paused contract - it MUST return 0. However, the current implementation returns a non-zero value on a paused contract. The current implementation ignores the fact that the contract might be paused and it returns the same value for both paused and unpaused contract.

Tools Used

Manual code review

Recommended Mitigation Steps

Detect that the contract is paused and return 0 when that scenario happens.

Assessed type

Other