code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L143-L145 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L181-L185 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L222-L226 https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L263-L267

Vulnerability details

Impact

When an asset is used with with decimals less than 18(1e6 for example) there is a possibility for the feeShare to be zero due to calculation that is rounding down. This means than fee recipient could lose out and the user can either get more shares(depositing) or receive more assets when withdrawing.

Proof of Concept

Whenever the assets or shares multiplied by the applicable vault fee is less than 1e18, the result of feeShares will be 0.

        uint256 feeShares = convertToShares(
            assets.mulDiv(uint256(fees.deposit), 1e18, Math.Rounding.Down)
        );

        uint256 feeShares = shares.mulDiv(
            depositFee,
            1e18 - depositFee,
            Math.Rounding.Down
        );

Tools Used

Manual review

Recommended Mitigation Steps

Enforce minimum amounts when setting up vaults and with deposit/mint/withdraw/redeem input parameters to ensure the result with be bigger than 1e18.

c4-sponsor commented 1 year ago

RedVeil marked the issue as sponsor confirmed

c4-judge commented 1 year ago

dmvt marked the issue as unsatisfactory: Invalid

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