code-423n4 / 2023-01-popcorn-findings

0 stars 2 forks source link

Upgraded Q -> 2 from #795 [1677634099280] #852

Closed c4-judge closed 1 year ago

c4-judge commented 1 year ago

Judge has assessed an item in Issue #795 as 2 risk. The relevant finding follows:

[04] VALUES OF fees ARE NOT CHECKED IN Vault.initialize FUNCTION When calling the following Vault.initialize function, the values of fees are not checked. It is possible that these fees are set to be above 1e18 when calling the Vault.initialize function; if this happens, calling functions like Vault.deposit can revert because the fee amount would exceed the asset amount. To avoid reverting these function calls, please consider checking the values of fees_ in the Vault.initialize function to ensure that these values are capped by some reasonable limits.

https://github.com/code-423n4/2023-01-popcorn/blob/main/src/vault/Vault.sol#L57-L98

function initialize(
    IERC20 asset_,
    IERC4626 adapter_,
    VaultFees calldata fees_,
    address feeRecipient_,
    address owner
) external initializer {
    ...
    fees = fees_;
    ...
}
c4-judge commented 1 year ago

dmvt marked the issue as duplicate of #396

c4-judge commented 1 year ago

dmvt marked the issue as partial-50

rbserver commented 1 year ago

Hi @dmvt,

It looks like that #845 is also upgraded from QA to M. This report and #845 both describe the possibility that these fees can be set to exceed 1e18 and offer similar mitigation suggestions. This report further describes the impact in which calling functions like Vault.deposit can revert, which is also described in #396 that is selected for report, but #845 does not describe such impact. Hence, this report is more similar to #396 while #845 is less. Yet, this report is marked as partial-50 but #845 has the full score. Hence, I would like to ask for the reason behind this.

Thanks again!

c4-judge commented 1 year ago

dmvt marked the issue as full credit

c4-judge commented 1 year ago

dmvt marked the issue as satisfactory

dmvt commented 1 year ago

Hi @dmvt,

It looks like that #845 is also upgraded from QA to M. This report and #845 both describe the possibility that these fees can be set to exceed 1e18 and offer similar mitigation suggestions. This report further describes the impact in which calling functions like Vault.deposit can revert, which is also described in #396 that is selected for report, but #845 does not describe such impact. Hence, this report is more similar to #396 while #845 is less. Yet, this report is marked as partial-50 but #845 has the full score. Hence, I would like to ask for the reason behind this.

Thanks again!

Fair. My original reasoning is that you didn't define "reasonable limits." Full credit restored.