Spectra - dimulski's results

Lines of code

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

Vulnerability details

Impact

There are several functions that don't follow the ERC-5095 standard, the redeem() and withdraw() with all of their variations: 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 as per the standard. Not following this part of the standard severely limits the functionality of the protocol. The maxRedeem() function MUST factor in both global and user-specific limits, like if redemption is entirely disabled (even temporarily) it MUST return 0. This is not the case as the _beforeRedeem() function which is called in redeem() has the pause modifier, and if redemption is paused temporarily after maturity maxRedeem() will be returning the maxBurnable value instead of 0. maxWithdraw() function has the same requirement as maxRedeem() as per the standard, and fails to follow it in the same manner.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Manual Review

Recommended Mitigation Steps

Follow the ERC5095 standard, for the redeem() and withdraw() functions consider the same implementation openzeppelin uses in their ERC4626 implementation

if (caller != owner) {
    _spendAllowance(owner, caller, shares);
}

Assessed type

Context