Spectra - Brenzee's results

Lines of code

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

Vulnerability details

Impact

PrincipalToken.sol does not comply with ERC5095 standard. This may DoS or return unexpected values for users that interact with the contract where users expect the contract to follow ERC5095 rules.

Proof of Concept

Two functions that do not comply with ERC5095 rules:

maxWithdraw(address) function

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

According to ERC5095

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

maxWithdraw function in PrincipalToken contract reverts when paused, but it has to return 0.

maxRedeem(address) function

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

According to ERC5095

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

maxRedeem function in PrincipalToken contract does not check if the contract is paused, but the function has to return 0 if the PrincipalToken is paused

Tools Used

Manual Review

Recommended Mitigation Steps

Make sure maxRedeem and maxWithdraw comply with ERC5095 rules.

Below are my suggestions to the maxWithdraw and maxRedeem functions maxWithdraw:

    function maxWithdraw(address owner) public view override returns (uint256) {
        return paused() ? 0 : convertToUnderlying(_maxBurnable(owner));
    }

maxRedeem:

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

Assessed type

Other