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

Non compliance with ERC standards #61

Closed
c4-bot-7 opened this issue Feb 28, 2024 · 8 comments
Closed

Non compliance with ERC standards #61

c4-bot-7 opened this issue Feb 28, 2024 · 8 comments
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-25 Incomplete articulation of vulnerability; eligible for partial credit only (25%) 🤖_04_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality

Comments

@c4-bot-7
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/main/src/tokens/PrincipalToken.sol#L483

Vulnerability details

The maxRedeem functions should return the 0 when the withdrawal is paused. But here, it's returning _maxBurnable(owner).

Proof of Concept

According to the ERC-5095 standards:

    MUST factor in both global and user-specific limits, like if redemption is entirely disabled (even temporarily) it MUST return 0.

But inside the Principal token contract, the function can be called even if the contract is paused.

File: PrincipalToken.sol

483:   function maxRedeem(address owner) public view override returns (uint256) {
           return _maxBurnable(owner);
       }

Tools Used

Manual Review & ERC-5095

Recommended Mitigation Steps

Add whenNotPaused modifier in maxRedeem function.

File: PrincipalToken.sol

483:   function maxRedeem(address owner) public view override whenNotPaused returns (uint256) {
           return _maxBurnable(owner);
       }

Assessed type

Error

@c4-bot-7 c4-bot-7 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 28, 2024
c4-bot-9 added a commit that referenced this issue Feb 28, 2024
@c4-bot-11 c4-bot-11 added the 🤖_04_group AI based duplicate group recommendation label Mar 1, 2024
@c4-pre-sort
Copy link

gzeon-c4 marked the issue as primary issue

@c4-pre-sort c4-pre-sort added the primary issue Highest quality submission among a set of duplicates label Mar 3, 2024
@c4-pre-sort
Copy link

gzeon-c4 marked the issue as duplicate of #33

@c4-pre-sort c4-pre-sort added duplicate-33 and removed primary issue Highest quality submission among a set of duplicates labels Mar 3, 2024
@c4-pre-sort
Copy link

gzeon-c4 marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Mar 3, 2024
@c4-judge
Copy link
Contributor

JustDravee marked the issue as partial-25

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

JustDravee marked the issue as satisfactory

@c4-judge c4-judge added duplicate-210 partial-25 Incomplete articulation of vulnerability; eligible for partial credit only (25%) 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-25

@Shubh0412
Copy link

@JustDravee
Wondering why this is partial-25 whereas issue #116 is partial-75?

@JustDravee
Copy link

@Shubh0412 because that person had several submissions under the duplicated finding and I totaled them

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-25 Incomplete articulation of vulnerability; eligible for partial credit only (25%) 🤖_04_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

6 participants