search

code-423n4 / 2024-02-spectra-findings

4 stars 2 forks source link

PrincipalToken.sol is not fully EIP-5095 compliant #168

Closed c4-bot-2 closed 8 months ago

c4-bot-2 commented 8 months ago

Lines of code

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

Vulnerability details

Impact

The sponsor mentioned in Main invariants that Principal Token is ERC5095, but PrincipalToken.sol is not fully EIP-5095 compliant, variation from the standard could break composability.

Proof of Concept

According to EIP-5095 method specifications (https://eips.ethereum.org/EIPS/eip-5095) For maxRedeem

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

    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;
        }
    }

Violates the following standard:

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

For maxWithdraw

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

Violates the following standards:

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

For previewWithdraw

    function previewWithdraw(
        uint256 assets
    ) external view override whenNotPaused returns (uint256) {
        uint256 ibts = IERC4626(ibt).previewWithdraw(assets);
        return previewWithdrawIBT(ibts);
    }

Violates the following standard:

  1. MUST NOT revert due to principal token contract specific user/global limits. MAY revert due to other conditions that would also cause withdraw to revert.

When PrincipalToken.sol is paused, maxRedeem and maxWithdraw should return 0, and all these functions should not revert.

Tools Used

Manual Review

Recommended Mitigation Steps

  1. maxRedeem and maxWithdraw should be updated to return 0 when contract is paused.
  2. Do not use the whenNotPaused modifier because that would cause a revert(maxRedeem and maxWithdraw should never revert according to EIP-5095).

Assessed type

Error

c4-pre-sort commented 8 months ago

gzeon-c4 marked the issue as duplicate of #33

c4-pre-sort commented 8 months ago

gzeon-c4 marked the issue as sufficient quality report

c4-judge commented 8 months ago

JustDravee marked the issue as partial-75

c4-judge commented 8 months ago

JustDravee marked the issue as satisfactory

c4-judge commented 7 months ago

JustDravee marked the issue as partial-75