Fujicracy / fuji-v2

Cross-chain money market aggregator
https://fuji-v2-frontend.vercel.app
15 stars 10 forks source link

CS - BaseVault unable to withdraw all assets and all shares #177

Closed 0xdcota closed 1 year ago

0xdcota commented 1 year ago

Where: https://github.com/Fujicracy/fuji-v2/blob/ab02d2308797577973ac358af8c7aadf973bcec7/packages/protocol/src/abstracts/BaseVault.sol#L161 https://github.com/Fujicracy/fuji-v2/blob/ab02d2308797577973ac358af8c7aadf973bcec7/packages/protocol/src/abstracts/BaseVault.sol#L332 https://github.com/Fujicracy/fuji-v2/blob/ab02d2308797577973ac358af8c7aadf973bcec7/packages/protocol/src/abstracts/BaseVault.sol#L177

Description: The vault compliant to ERC4626 uses maxRedeem and maxWithdraw functions to calculate the maximum shares and deposits to be redeemed and withdrawn, respectively. However, these functions use the conversion functions that cause the arithmetic errors and when the user tries to withdraw all assets or redeem all shares (returned by maxWithdraw and balanceOf functions, respectively) the transaction reverts. This bug happens only when more than one user deposits to the vault and is caused by the fact that when withdrawing all available assets or shares, function maxRedeem is called and it does the rounding down (BaseVault.sol#L177) when converting to the redeemable shares and returning an amount smaller by 1 than the actual share balance. When a user tries to redeem all shares using the maxRedeem function, the transaction is not reverted but the user is left with 1 share in the vault. The severity of this issue has been increased to Medium due to the fact that protocol must allow the withdrawal of the amount of assets returned by maxWithdraw function.

Recommendation: ● The bug is caused by the rounding in conversion functions. Consider using one type of rounding for all operations. ● Also, add tests for cases when multiple users use vaults.