Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Users holding eip-20 approval cannot redeem or withdraw their funds. #89

Closed
c4-bot-4 opened this issue Feb 29, 2024 · 5 comments
Closed
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-210 partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) sufficient quality report This report is of sufficient quality

Comments

@c4-bot-4
Copy link
Contributor

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

Vulnerability details

Impact

Incompatibility with eip-5095 will result in the integrator not being able to redeem or withdraw funds in accordance with the standard integration.

Proof of Concept

In the documentation, we can see that redeem and withdraw are compliant with the eip-5095 standard.
In the redeem and withdraw methods, the standard requires:
https://eips.ethereum.org/EIPS/eip-5095#redeem

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. MAY support an additional flow in which the principal tokens are transferred to the Principal Token contract before the redeem execution, and are accounted for during redeem.

In the code we can see that if the owner is not equal to msg.sender it REVERTS.
https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L829-L831

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

And according to the standard requirements, when the owner is not equal to msg.sender, it should be performed

_spendAllowance(owner, msg.sender, value); 

in order to satisfy the standard requirement

You can refer to the reference implementation provided by the standard

Tools Used

Manual Review

Recommended Mitigation Steps

1._beforeRedeem can be modified to:
if (_owner != msg.sender) {
_spendAllowance(_owner, msg.sender, _shares);
}
2._beforeWithdraw remove _owner is not equal to msg.sender judgment, add to the
_withdrawShares method.

Assessed type

Other

@c4-bot-4 c4-bot-4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Feb 29, 2024
c4-bot-7 added a commit that referenced this issue Feb 29, 2024
@c4-pre-sort
Copy link

gzeon-c4 marked the issue as duplicate of #33

@c4-pre-sort c4-pre-sort added duplicate-33 sufficient quality report This report is of sufficient quality labels Mar 3, 2024
@c4-pre-sort
Copy link

gzeon-c4 marked the issue as sufficient quality report

@c4-judge c4-judge added the partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) label Mar 11, 2024
@c4-judge
Copy link
Contributor

JustDravee marked the issue as partial-50

@c4-judge c4-judge added satisfactory satisfies C4 submission criteria; eligible for awards and removed partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) labels Mar 11, 2024
@c4-judge
Copy link
Contributor

JustDravee marked the issue as satisfactory

@c4-judge c4-judge added duplicate-210 partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) and removed duplicate-33 satisfactory satisfies C4 submission criteria; eligible for awards labels Mar 11, 2024
@c4-judge
Copy link
Contributor

JustDravee marked the issue as partial-50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-210 partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

3 participants