Users can burn YT when `PrincipalToken` contract is paused by

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/YieldToken.sol#L58-L61

Vulnerability details

When the PrincipalToken contract has been paused (_paused set to true), users should NOT be able to burn YT tokens. This can be inferred as the _beforeWithdraw() function has the whenNotPaused and it is called before every single withdraw (or the next call in the stack). In other words withdraws are paused. Also during withdraw the yield is updated and YT tokens are burned (only in _beforeRedeem).

    function _beforeRedeem(uint256 _shares, address _owner) internal nonReentrant whenNotPaused {
        if (_owner != msg.sender) {
            revert UnauthorizedCaller();
        }
        if (_shares > _maxBurnable(_owner)) {
            revert UnsufficientBalance();
        }
        if (block.timestamp >= expiry) {
            if (!ratesAtExpiryStored) {
                storeRatesAtExpiry();
            }
        } else {
            updateYield(_owner);
            IYieldToken(yt).burnWithoutUpdate(_owner, _shares);
        }
        _burn(_owner, _shares);
    }

There is also another way for a user to update his yield and burn tokens which is through YieldToken::burn()

    function burn(uint256 amount) public override {
        IPrincipalToken(pt).updateYield(msg.sender);
        _burn(msg.sender, amount);
    }

However this function does not check if the protocol is paused.

Impact

Users are able to burn tokens when the system is paused, which they should not be able to do. This could be a problem if the protocol ever needs to temporarily pause for upgrade or the functionality has a bug.

Proof of Concept

Add the following test to PrincipalToken.t.sol and run with forge test --match-test testBurn -vvv

    function testBurnWhenPaused() public {
        uint256 amountToDeposit = 100e18;
        _testDeposit(amountToDeposit, address(this));

        uint256 amountToBurn = yt.actualBalanceOf(address(this)) / 2; // burn half
        uint256 ytBalanceBefore = yt.actualBalanceOf(address(this));

        vm.expectEmit(false, false, false, true); // verify paused
        emit Paused(scriptAdmin);
        vm.prank(scriptAdmin);
        principalToken.pause();

        yt.burn(amountToBurn);
        uint256 ytBalanceAfter = yt.actualBalanceOf(address(this));

        assertEq(
            ytBalanceBefore,
            ytBalanceAfter + amountToBurn
        );
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Add whenNotPaused modifier to PrincipalToken::updateYield. Optional: Remove it from _beforeRedeem and _beforeWithdraw as both methods call updateYield.

Assessed type

Other

[c4-pre-sort]

gzeon-c4 marked the issue as duplicate of #114

[c4-pre-sort]

gzeon-c4 marked the issue as not a duplicate

[c4-pre-sort]

gzeon-c4 marked the issue as sufficient quality report

[c4-pre-sort]

gzeon-c4 marked the issue as duplicate of #7

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid