search

code-423n4 / 2024-02-spectra-findings

4 stars 2 forks source link

PrincipalToken not compliant with EIP-5095 #263

Closed c4-bot-4 closed 8 months ago

c4-bot-4 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L806-L808 https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L829-L831 https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L460

Vulnerability details

Impact

PrincipalToken not compliant with EIP-5095. This can render unusable external integrations.

Proof of Concept

  1. withdraw 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. But contract allows only owner to withdraw
  2. Same for redeem (code)
  3. maxRedeem MUST not revert, but it reverts in case of protocol pause

Tools Used

Manual review

Recommended Mitigation Steps

  1. Remove owner check in _beforeRedeem/Withdraw and add following lines (adapted from openzeppelin ERC4626): 
    if (msg.sender != owner) {
_spendAllowance(owner, caller, shares);
}
  2. Remove whenNotPaused modifier from maxWithdraw

Assessed type

Other

c4-pre-sort commented 8 months ago

gzeon-c4 marked the issue as duplicate of #33

c4-pre-sort commented 8 months ago

gzeon-c4 marked the issue as sufficient quality report

c4-judge commented 7 months ago

JustDravee marked the issue as satisfactory

c4-judge commented 7 months ago

JustDravee marked the issue as partial-75