PrincipalToken is not ERC-5095 compliant

[c4-bot-7]

Lines of code

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 https://github.com/code-423n4/2024-02-spectra/blob/main/src/tokens/PrincipalToken.sol#L278-L287 https://github.com/code-423n4/2024-02-spectra/blob/main/src/tokens/PrincipalToken.sol#L229-L237

Vulnerability details

Impact

Protocols that try to integrate with Spectra, expecting PrincipalToken to be ERC-5095 compliant, will face an array of issues that may damage Spectra's brand and limit Spectra's growth in the market.

Proof of Concept

All official ERC-5095 requirements are on their official page. Non-compliant methods are listed below along with why they are not compliant and code POCs demonstrating the issues. To run the POCs, copy-paste them into PrincipalToken.t.sol:

PrincipalToken::redeem and PrincipalToken::withdraw

As specified in ERC-5095, both withdraw and redeem must support a flow where msg.sender has approval over the owner's tokens.

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.

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.

However, neither PrincipalToken::redeem nor PrincipalToken::withdraw support this flow type.

    //copy-paste into `PrincipalToken.sol`
    function testRedeemDoesNotSupportERC20ApprovalFlow() public {
        uint256 amountToDeposit = 1e18;
        uint256 expected = _testDeposit(amountToDeposit, address(this));
        _increaseTimeToExpiry();
        principalToken.storeRatesAtExpiry();

        principalToken.approve(MOCK_ADDR_5, UINT256_MAX);
        assertEq(principalToken.allowance(address(this), MOCK_ADDR_5), UINT256_MAX);

        vm.startPrank(MOCK_ADDR_5);
        vm.expectRevert();
        //Should not revert as MOCK_ADDR_5 has allowance over tokens.
        principalToken.redeem(expected, MOCK_ADDR_5, address(this));
        vm.stopPrank();
    }

    function testWithdrawDoesNotSupportERC20ApprovalFlow() public {
        uint256 amount = 1e18;
        underlying.approve(address(principalToken), amount);
        principalToken.deposit(amount, testUser);

        principalToken.approve(MOCK_ADDR_5, UINT256_MAX);
        assertEq(principalToken.allowance(address(this), MOCK_ADDR_5), UINT256_MAX);

        vm.prank(MOCK_ADDR_5);
        vm.expectRevert();
        //Should not revert as MOCK_ADDR_5 has allowance over tokens.
        principalToken.withdraw(amount, MOCK_ADDR_5, testUser);

        vm.stopPrank();
    }

PrincipalToken::maxWithdraw

According to ERC-5095, maxWithdraw must not revert and must return 0 if withdrawal is disabled.

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

MUST NOT revert.

However, PrincipalToken::maxWithdraw reverts if PrincipalToken is paused.

   //copy-paste into `PrincipalToken.sol`
   function testMaxWithdrawRevertsWhenPausedWhenItShouldNeverRevert() public {
        vm.prank(scriptAdmin);
        principalToken.pause();

        vm.expectRevert();
        //Should not revert, should return 0 to comply to ERC-5095.
        principalToken.maxWithdraw(address(this));
    }

PrincipalToken::maxRedeem

According to ERC-5095, maxRedeem must return 0 if redeem is disabled.

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

However PrincipalToken::maxRedeem does not return 0 when PrincipalToken is paused.

    //copy-paste into `PrincipalToken.sol`
    function testMaxRedeemDoesNotReturnZeroWhenPausedEvenThoughItShould() public {
        uint256 amountToDeposit = 1e18;
        _testDeposit(amountToDeposit, address(this));

        vm.prank(scriptAdmin);
        principalToken.pause();

        //Should return 0 to comply to ERC-5095.
        assertNotEq(principalToken.maxRedeem(address(this)), 0);
    }

Tools Used

Manual review.

Recommended Mitigation Steps

• PrincipalToken::redeem and PrincipalToken::withdraw should be changed to support a flow where msg.sender has EPI-20 approval over the owner's principal tokens.
• PrincipalToken::maxRedeem and PrincipalToken::maxWithdrawshould be changed to return 0 when PrincipalToken is paused.

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 selected for report

[c4-judge]

JustDravee changed the severity to 2 (Med Risk)

[c4-sponsor]

yanisepfl (sponsor) confirmed