Submitted by KingNFT, also found by immeas and bin2chen
The calculation of takeFees in Vault contract is incorrect, which will cause fee charged less than expected.
https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L480-L494
To simplify the problem, lets given the fee parameters are as follows:
And the initial asset token and share tokens in the vault are:
The yield earnings is also set to 0.
As the yearly management fee is 50%, so the expected fee for one year is:
Now lets calculate the actual fee.
The implementation of accruedManagementFee() is
So in this case, one year later, the calculation of first step for managementFee will be
The implementation of takeFees() is
So, variables before L491 of takeFees() will be
As the implementation of convertToShares() is
So the second parameter for _mint() call at L491 is
After _mint() at L491, the variables will be
The implementation of convertToAssets() is
Now we can get actual fee by calling convertToAsset(), which is
We can see the actual fee is less than expected, the realistic parameters will be less than the give 0.5e18, but it will be always true that the actual fee charged is not enough.
VS Code
Use the correct formula such as:
RedVeil (Popcorn) confirmed 
