Platform: Code4rena
Start Date: 23/02/2024
Pot Size: $36,500 USDC
Total HM: 2
Participants: 39
Period: 7 days
Judge: Dravee
Id: 338
League: ETH
Rank: 32/39
Findings: 1
Award: $53.72
🌟 Selected for report: 0
🚀 Solo Findings: 0
53.7155 USDC - $53.72
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
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
Manual Review
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.
ERC4626
#0 - c4-pre-sort
2024-03-03T09:24:40Z
gzeon-c4 marked the issue as duplicate of #33
#1 - c4-pre-sort
2024-03-03T14:08:29Z
gzeon-c4 marked the issue as sufficient quality report
#2 - c4-pre-sort
2024-03-03T14:08:43Z
gzeon-c4 marked the issue as not a duplicate
#3 - c4-pre-sort
2024-03-03T14:08:52Z
gzeon-c4 marked the issue as duplicate of #33
#4 - c4-judge
2024-03-11T00:04:06Z
JustDravee marked the issue as not a duplicate
#5 - JustDravee
2024-03-11T00:09:55Z
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.
#6 - c4-judge
2024-03-11T00:10:03Z
JustDravee marked the issue as satisfactory
#7 - c4-sponsor
2024-03-11T17:38:14Z
yanisepfl (sponsor) disputed
#8 - yanisepfl
2024-03-11T17:42:40Z
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: https://github.com/code-423n4/2024-02-spectra-findings/issues/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 https://github.com/code-423n4/2024-02-spectra-findings/issues/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.
#9 - c4-judge
2024-03-11T19:49:07Z
JustDravee marked the issue as unsatisfactory: Invalid
#10 - 0xhacksmithh
2024-03-12T18:08:11Z
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.
#11 - c4-judge
2024-03-14T12:48:35Z
JustDravee marked the issue as duplicate of #210
#12 - c4-judge
2024-03-14T12:48:40Z
JustDravee marked the issue as partial-50
#13 - JustDravee
2024-03-14T12:50:35Z
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.