The ERC4626 standard is not followed correctly

[c4-bot-10]

Lines of code

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

Vulnerability details

Description

As per EIP-4626, the maxDeposit method "MUST factor in both global and user-specific limits, like if deposits are entirely disabled (even temporarily) it MUST return 0.". This is not the case currently, as even if the contract is paused, the maxDeposit method will still return what it usually does.

    function maxDeposit(address) external pure override returns (uint256) { 
        return type(uint256).max;
    }

maxMint
maxWithdraw
maxRedeem
All of the above functions should return 0 when their respective functions are disabled

Proof of Concept

Tools Used

Manual Review

Recommended Mitigation Steps

All functions listed above should be modified to meet the specifications of EIP-4626.
Go through the EIP-4626 standard and follow it for all methods that override methods from the inherited ERC4626 implementation.

Assessed type

ERC4626

[c4-pre-sort]

gzeon-c4 marked the issue as duplicate of #33

[c4-pre-sort]

gzeon-c4 marked the issue as sufficient quality report

[c4-pre-sort]

gzeon-c4 marked the issue as not a duplicate

[c4-pre-sort]

gzeon-c4 marked the issue as duplicate of #33

[c4-judge]

JustDravee marked the issue as not a duplicate

[JustDravee]

Although it may seem similar to all the others about the compliance to EIP5095, this is the only one which mentioned the rule on maxDeposit and focusing on EIP4626.
However, there's no maxMint here.
maxWithdraw and maxRedeem are dealt with through EIP5095 too.

[c4-judge]

JustDravee marked the issue as satisfactory

[c4-sponsor]

yanisepfl (sponsor) disputed

[yanisepfl]

As much as we want to follow erc4626 as much as possible, it is not in our specs, as opposed to erc5095 which is why we confirmed: #210.

Also, the comment on maxDeposit is indeed a good recommendation that we will keep in mind, but it is very close to the part on the view functions maxRedeem and maxWithdraw when our contracts are paused here #210, which is also more complete than here.

Finally, the fact that we do not implement ERC4626's mint method and thus maxMint, was not caught by the auditor.

For those reasons, we dispute this issue.

[c4-judge]

JustDravee marked the issue as unsatisfactory:
Invalid

[0xhacksmithh]

I don't understand why this whole finding get invalidated.

In Description section i mentioned that maxRedeem and maxWithdraw should return 0 when their respective functions are disabled. These are similar in case of both EIPs 4626 & 5059 (maxRedeem) as mentioned in Issue- 210

And recommendation on maxDeposit liked by Sponcer

i know i mistakely mentioned maxMint its due to my bugs collection notes.

if i think 3 of them are still valid(partially)(if i submited them individually). like this QA

Waiting for your final judgment, sorry if this was a childish comment, thanks for your time.

[c4-judge]

JustDravee marked the issue as duplicate of #210

[c4-judge]

JustDravee marked the issue as partial-50

[JustDravee]

Hi @0xhacksmithh
It was a bit harsh and I agree that in the end, fixing the root of what you mentioned would've spilled over partially to the lack of 5095 compliance, even if unintended.
Putting it back as duplicate due to that.