maxWithdraw implementation is not EIP-5095 compliant

[c4-bot-7]

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:

• ## vMaia is an ERC-4626 compliant but maxWithdraw & maxRedeem functions are not fully up to EIP-4626's specification 2023-05-maia-findings#585

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

• Vault.sol is not EIP-4626 compliant  2022-09-y2k-finance-findings#47

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

• (Lack of EIP-4626 compliance) Vault.sol is not EIP-4626 compliant  2022-09-y2k-finance-findings#47 - has been evaluated as High
• (Lack of EIP-4524 compliance) HolographERC20 breaks composability by forcing usage of draft proposal EIP-4524 2022-10-holograph-findings#440 - has been evaluated as Medium
• (Lack of EIP-721 compliance) HolographERC721.safeTransferFrom not compliant with EIP-721 2022-10-holograph-findings#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

[c4-pre-sort]

gzeon-c4 marked the issue as duplicate of #33

[c4-pre-sort]

gzeon-c4 marked the issue as sufficient quality report

[c4-judge]

JustDravee marked the issue as partial-50

[c4-judge]

JustDravee marked the issue as satisfactory

[c4-judge]

JustDravee marked the issue as partial-50