The owner of shares is not verified in the withdraw functions. This could allow anyone to burn someone else's shares via withdraw.

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L290-L300

Vulnerability details

Impact

It allows anyone to maliciously burn someone else's shares by calling the withdraw functions. Specifically:

The withdraw and withdrawIBT functions allow burning an arbitrary amount of shares belonging to any user by simply specifying their address as the owner parameter. This could drain all the value from unsuspecting users by improperly redeeming their shares. Since shares entitle holders to the underlying assets in the vault, being able to improperly burn someone's shares causes them to lose the value they represent. The severity is high because it violates the basic expectation that only share owners can redeem their own shares. Allowing anyone to burn anyone else's shares completely compromises ownership control. This vulnerability enables theft of funds and should be fixed immediately.

Overall this is a critical severity issue since it leads to direct loss of funds for users. It breaks a core security expectation of non-fungible token ownership in the contract. The impact cannot be overstated - it makes the shares stealable and valueless as valid ownership credentials.

Proof of Concept

The withdraw functions do not verify that the msg.sender is the owner of the shares being withdrawn. This is a vulnerability that could allow anyone to burn someone else's shares. Here is how it works: The withdraw() and withdrawIBT() functions take an owner parameter, which is intended to be the owner of the shares. However, there is no verification that msg.sender equals this owner parameter. For example:

   function withdraw(
       uint256 assets,
       address receiver, 
       address owner
   ) public override returns (uint256 shares) {

       _beforeWithdraw(assets, owner);

       // Burn shares based on assets and send assets to receiver
       // No check that msg.sender == owner
   }

This means anyone can call withdraw and pass any arbitrary address as the owner. That address's shares will be burned and assets sent to the receiver. So the impact is that anyone can burn anyone else's shares, effectively stealing their deposited assets.

1. Alice deposits 100 assets and has 100 shares
2. Eve calls withdraw(50 assets, Eve's address, Alice's address)
3. 50 of Alice's shares are burned and 50 assets sent to Eve
4. Eve has stolen 50 assets by burning Alice's shares

Tools Used

Manual

Recommended Mitigation Steps

There should be a check that msg.sender equals the owner in both withdraw functions before burning any shares:

   function withdraw(
       uint256 assets,
       address receiver, 
       address owner
   ) public override returns (uint256 shares) {

        if (msg.sender != owner) {
           revert UnauthorizedCaller(); 
       }

       _beforeWithdraw(assets, owner);

       // Rest of implementation
   }

Assessed type

Other

[c4-pre-sort]

gzeon-c4 marked the issue as insufficient quality report

[gzeon-c4]

checked in _beforeWithdraw

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid