Spectra - btk's results

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