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

maxWithdraw() will revert when paused but should return 0 instead #118

Closed
c4-bot-4 opened this issue Feb 29, 2024 · 5 comments
Closed

maxWithdraw() will revert when paused but should return 0 instead #118

c4-bot-4 opened this issue Feb 29, 2024 · 5 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-75 Incomplete articulation of vulnerability; eligible for partial credit only (75%) 🤖_04_group AI based duplicate group recommendation 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/25603ac27c3488423a0739b66e784c01a3db7d75/src/tokens/PrincipalToken.sol#L461-L463

Vulnerability details

Vulnerability explanation

According to the almighty EIP-5095, the maxWithdraw() function "MUST return the maximum amount of underlying tokens that could be redeemed from holder through withdraw and not cause a revert". However instead of this the cheeky maxWithdraw() function in the contract will revert when paused because of the whenNotPaused modifier

Impact

The function reverts when the redemptions are paused, which is not in line with the requirements of the EIP-5095.

Proof of Concept

The maxWithdraw()

    function maxWithdraw(address owner) public view override whenNotPaused returns (uint256) {
        return convertToUnderlying(_maxBurnable(owner)); //@note inclusive of fees
    }

reverts when the DAO decides to paused the contract. If the function is called while paused the function reverts.

    function _requireNotPaused() internal view virtual {
        if (paused()) {
 @>          revert EnforcedPause();
        }
    }

Tools Used

EIP-5095 docs

Recommended Mitigation Steps

If the contract happens to be paused, return 0 instead of revertin'.

   -- function maxWithdraw(address owner) public view override whenNotPaused returns (uint256) {
     
   ++ function maxWithdraw(address owner) public view override returns (uint256) {
 ++    if(paused()) return 0;
       return convertToUnderlying(_maxBurnable(owner)); //@note inclusive of fees
    }

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-8 added a commit that referenced this issue Feb 29, 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 duplicate of #33

@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 c4-judge added the partial-75 Incomplete articulation of vulnerability; eligible for partial credit only (75%) label Mar 11, 2024
@c4-judge
Copy link
Contributor

JustDravee marked the issue as partial-75

@c4-judge
Copy link
Contributor

JustDravee marked the issue as satisfactory

@c4-judge c4-judge added satisfactory satisfies C4 submission criteria; eligible for awards duplicate-210 partial-75 Incomplete articulation of vulnerability; eligible for partial credit only (75%) and removed partial-75 Incomplete articulation of vulnerability; eligible for partial credit only (75%) 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-75

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-75 Incomplete articulation of vulnerability; eligible for partial credit only (75%) 🤖_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

4 participants