Lif3 - vaults - SC

In function `deposit()`, when calculating deposit share amount, the code calls function `_calculateShare()` with input `_amount` and `_lockTime`. The `_amount` it is using is the full amount of deposited token before fee deduction. If `depositFee` is larger than 0, then the actual deposited amount should be less than the full amount. The user share amount is calculated again during withdrawal, and this time the actual deposited amount is used. As a result, the withdrawn share amount would always be smaller than deposited. This may cause a leftover of rewards in the contract. ```solidity=352 if (depositFee > 0) { uint256 depositFeeAmount = (_amount * depositFee) / 10000; uint256 _amountMinusFee = _amount - depositFeeAmount; depositToken.safeTransferFrom(sender, feeCollector, depositFeeAmount); depositToken.safeTransferFrom(sender, address(this), _amountMinusFee); finalAmount = _amountMinusFee; } else { depositToken.safeTransferFrom(sender, address(this), _amount); finalAmount = _amount; } uint256 share = _calculateShare(_amount, _lockTime); ```

A reentrancy attack can occur when the contract creates a function that makes an external call to another untrusted contract before resolving any effects. If the attacker can control the untrusted contract, they can make a recursive call back to the original function, repeating interactions that would have otherwise not run after the external call resolved the effects. 1. In function `deposit()`, the transfer-in of deposit token happens before updating state variables. 2. In function `withdraw()`, the transfer-out of deposit token and reward tokens happen before updating state variables. 3. In function `emergencyWithdraw()`, the transfer of reward tokens happen before updating state variables.

Performing integer division before multiplication truncates the low bits, losing the precision of calculation. ```solidity=218 return lock.amount * (lock.finalClaimEpochStartTime + epochLength - taxCalculationStart) / lock.lockTime * emergencyWithdrawTax / 10000; ``` --- ```solidity=338 PRBMath.UD60x18 memory exponent = PRBMath.UD60x18({value: _lockTime/epochLength*1e18}); ``` --- ```solidity=452 emergencyWithdrawTaxAmount = amount * (lock.finalClaimEpochStartTime + epochLength - currentEpochStart) / lock.lockTime * emergencyWithdrawTax / 10000; ```

In contract LockNFT, function `destroyLock()` and `_destroyLock()` can be called by any `DESTROYER_ROLE` to destroy any lock NFT token. This makes it possible for a user to make deposit in one reward pool, and call `withdraw()` or `emergencyWithdraw()` in another pool. As a result, the user's LockNFT would be burned, and he wouldn't be able to withdraw his deposited assets.

The linked variables assigned in the constructor can be declared as `immutable`. Immutable state variables can be assigned during contract creation but will remain constant throughout the lifetime of a deployed contract. A big advantage of immutable variables is that reading them is significantly cheaper than reading from regular state variables since they will not be stored in storage.