The Principal Token is not ERC-5095 compliant

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/main/src/tokens/PrincipalToken.sol#L806-L808
https://github.com/code-423n4/2024-02-spectra/blob/main/src/tokens/PrincipalToken.sol#L483-L485
https://github.com/code-423n4/2024-02-spectra/blob/main/src/tokens/PrincipalToken.sol#L460-L462

Vulnerability details

Impact

The Principal Token deviate from the ERC-5095 specification which may break external integrations.

Proof of Concept

ERC-5095 stipulates:

MUST support a redeem flow where the Principal Tokens are burned from holder directly where holder is msg.sender or msg.sender has EIP-20 approval over the principal tokens of holder.

However, the PrincipalToken.redeem function calls the _beforeRedeem function:

    function _beforeRedeem(uint256 _shares, address _owner) internal nonReentrant whenNotPaused {
        if (_owner != msg.sender) {
            revert UnauthorizedCaller();
        }
    }    

This reverts if the owner is not the sender, thus violating ERC5095.

ERC-5095 also states:

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

As previously explained, the PrincipalToken.redeem calls the _beforeRedeem function, which has a whenNotPaused modifier, meaning redemption can be paused by the owner.

However, the maxRedeem function will return maxBurnable of the user if redemption is paused:

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

It's important to note the same issue exists in the maxWithdraw function, as it will still call convertToUnderlying with the user's maxBurnable even if the contract is paused. maxWithdraw has a whenNotPaused modifier, which will revert, although the ERC clearly states:

MUST NOT revert.

Tools Used

Manual review

Recommended Mitigation Steps

Consider following the ERC-5095 specifications.

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

[0xbtk]

Hey @JustDravee, I believe I've mentioned all the lack of compliance, could you please elaborate on why this was marked partial?