PrincipalToken is not fully EIP5095 compliant

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L460
https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L466
https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L493
https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L229-L275
https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L278-L326

Vulnerability details

Impact

1. maxWithdraw, maxWithdrawIBT, previewWithdrawIBT and previewWithdraw must not revert, but are pausable, causing them to revert when the contract is paused.

2. The redeem functions must be callable by the owner or a user approved by the owner

3. The withdraw functions must be callable by the owner or a user approved by the owner

Proof of Concept

The PrincipalToken contract aims to be fully EIP5095 compliant, but isn't fully compliant, breaking composability.

1. maxWithdraw, maxWithdrawIBT, previewWithdrawIBT and previewWithdraw must not revert

These functions hold a whenNotPaused modifier, causing them to revert when the contract is paused.

    /** @dev See {IPrincipalToken-maxWithdraw}.
     */
    function maxWithdraw(address owner) public view override whenNotPaused returns (uint256) {
        return convertToUnderlying(_maxBurnable(owner));
    }

    /** @dev See {IPrincipalToken-maxWithdrawIBT}.
     */
    function maxWithdrawIBT(address owner) public view override whenNotPaused returns (uint256) {
        return _convertSharesToIBTs(_maxBurnable(owner), false);
    }

According to the ERC5095 spec,

maxWithdraw

Maximum amount of the underlying asset that can be redeemed from the holder principal token balance, through a withdraw call.

...

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

MUST NOT revert.

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

    /** @dev See {IPrincipalToken-previewWithdrawIBT}. */
    function previewWithdrawIBT(uint256 ibts) public view override whenNotPaused returns (uint256) {
        return _convertIBTsToShares(ibts, true);
    }

According to the ERC5095 spec,

previewWithdraw

Allows an on-chain or off-chain user to simulate the effects of their withdrawal at the current block, given current on-chain conditions.

...

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.

---

2. The redeem functions must be callable by the owner or a user approved by the owner

   function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public override returns (uint256 assets) {
        _beforeRedeem(shares, owner);
...
    }

    /** @dev See {IPrincipalToken-redeem}. */
    function redeem(
        uint256 shares,
        address receiver,
        address owner,
        uint256 minAssets
    ) external override returns (uint256 assets) {
        assets = redeem(shares, receiver, owner);
...
    }

    /** @dev See {IPrincipalToken-redeemForIBT}. */
    function redeemForIBT(
        uint256 shares,
        address receiver,
        address owner
    ) public override returns (uint256 ibts) {
        _beforeRedeem(shares, owner);
...
    }

    /** @dev See {IPrincipalToken-redeemForIBT}. */
    function redeemForIBT(
        uint256 shares,
        address receiver,
        address owner,
        uint256 minIbts
    ) external override returns (uint256 ibts) {
        ibts = redeemForIBT(shares, receiver, owner);
...
    }

These functions call the _beforeRedeem function, which burns the shares from the owner. Note that the function reverts if msg.sender is not the owner.

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

Hence, an approved user cannot redeem on behalf of the owner.

But according to the spec,

redeem

At or after maturity, burns exactly principalAmount of Principal Tokens from from and sends underlyingAmount of underlying tokens to to.

...

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. MAY support an additional flow in which the principal tokens are transferred to the Principal Token contract before the redeem execution, and are accounted for during redeem.

---

3. The withdraw functions must be callable by the owner or a user approved by the owner

_beforeWithdraw reverts but withdrawIBT and withdraw should be performable by owner or approved owner

 function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) public override returns (uint256 shares) {
        _beforeWithdraw(assets, owner);
        (uint256 _ptRate, uint256 _ibtRate) = _getPTandIBTRates(false);
        uint256 ibts = IERC4626(ibt).withdraw(assets, receiver, address(this));
        shares = _withdrawShares(ibts, receiver, owner, _ptRate, _ibtRate);
    }

    /** @dev See {IPrincipalToken-withdraw}. */
    function withdraw(
        uint256 assets,
        address receiver,
        address owner,
        uint256 maxShares
    ) external override returns (uint256 shares) {
        shares = withdraw(assets, receiver, owner);
...
    }

    /** @dev See {IPrincipalToken-withdrawIBT}. */
    function withdrawIBT(
        uint256 ibts,
        address receiver,
        address owner
    ) public override returns (uint256 shares) {
        _beforeWithdraw(IERC4626(ibt).previewRedeem(ibts), owner);
        (uint256 _ptRate, uint256 _ibtRate) = _getPTandIBTRates(false);
        shares = _withdrawShares(ibts, receiver, owner, _ptRate, _ibtRate);
...
    }

    /** @dev See {IPrincipalToken-withdrawIBT}. */
    function withdrawIBT(
        uint256 ibts,
        address receiver,
        address owner,
        uint256 maxShares
    ) external override returns (uint256 shares) {
        shares = withdrawIBT(ibts, receiver, owner);
...
    }

These functions all call the _beforeWithdraw function, which burns the shares from the owner. Note that the function reverts if msg.sender is not the owner.

    function _beforeWithdraw(uint256 _assets, address _owner) internal whenNotPaused nonReentrant {
        if (_owner != msg.sender) { //@note
            revert UnauthorizedCaller();
...

Hence, an approved user cannot withdraw on behalf of the owner.

But according to the spec,

withdraw

Burns principalAmount from holder and sends exactly underlyingAmount of underlying tokens to receiver.

...

MUST support a withdraw 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. MAY support an additional flow in which the principal tokens are transferred to the principal token contract before the withdraw execution, and are accounted for during withdraw.

Tools Used

Manual code review

Recommended Mitigation Steps

1. maxWithdraw, maxWithdrawIBT, previewWithdrawIBT and previewWithdraw must not revert

Remove the whenNotPaused modifier and for the maxWithdraw/maxWithdrawIBT function, return 0 when paused.

2. The redeem functions must be callable by the owner or a user approved by the owner

Refactor the check for unauthorized caller in the _beforeRedeem function to include approved holders. Include also a check that allowance => shares to redeem.

3. The withdraw functions must be callable by the owner or a user approved by the owner

Refactor the check for unauthorized caller in the _beforeWithdraw function to include approved holders. Include also a check that allowance => shares to redeem.

Assessed type

Other

[c4-pre-sort]

gzeon-c4 marked the issue as primary issue

[c4-pre-sort]

gzeon-c4 marked the issue as high quality report

[c4-sponsor]

yanisepfl (sponsor) confirmed

[yanisepfl]

This is a good catch, thanks!

[c4-judge]

JustDravee marked the issue as satisfactory

[c4-judge]

JustDravee marked the issue as partial-75

[c4-judge]

JustDravee marked issue #210 as primary and marked this issue as a duplicate of 210

[JustDravee]

Hi @ZanyBonzy ,

Basically, wardens more or less grouped a certain amount of lack of compliance and I went with numbers.

I counted yours as having 2 categories of them: those related to maxWithdraw() reverting due to the pause (encompassing previewWithdraw() even if not mentioned because I'd expect Spectra to mirror the new logic "obviously"), and those related to the lack of user approved by the owner capabilities.

But here, yours failed to mentioned that maxRedeem isn't returning 0 if redeem is disabled. Hence 75%. Issue 212 mentioned it, but forgot the lack of approved users capabilities, so it got 75% too. Thank you for mentioning 61, as you could see in the history, it had 25%, but somehow the label got removed (not intentionally, I'll make another pass to see if this happened elsewhere). I applied 25% again, because it only had the "maxRedeem" bug.

Again, your report is indeed of high quality, and it wasn't easy finding a fair solution here, given the different mixes of different lack of compliance issues.