Calm period not checked in all functions
Description
The onlyCalmPeriods modifier is a critical safety component of the strategy, as it is used to prevent deposits and harvests when the current price is not within 1% of the time-weighted average price (TWAP) over the last 60 seconds. This is to protect against price manipulation by an attacker during their transaction, possibly using flash loans or other means.
/// @notice Modifier to only allow deposit/harvest actions when current price is within 1% of twap.
modifier onlyCalmPeriods() {
int24 tick = currentTick();
int56 twapTick = twap();
int56 upperBound = deviation + deviationThreshold;
int56 lowerBound = deviationThreshold - deviation;
// Calculate if greater than deviation % from twap and revert if it is.
if (tick >= 0) {
if (tick > twapTick * upperBound / deviationThreshold || tick < twapTick * lowerBound / deviationThreshold) revert NotCalm();
} else if (tick < twapTick * upperBound / deviationThreshold || tick > twapTick * lowerBound / deviationThreshold) revert NotCalm();
_;
}One of the harvest functions as well as withdraw do not check whether the system is in a calm period or not. This means that users can still interact with the system, even though it is not safe to do so. This can lead to unexpected behavior and potential losses for the users.
function harvest() external {
_harvest(tx.origin);
}
function withdraw(uint256 _amount0, uint256 _amount1) external {
_onlyVault();
// Liquidity has already been removed in beforeAction() so this is just a simple withdraw.
if (_amount0 > 0) IERC20Metadata(lpToken0).safeTransfer(vault, _amount0);
if (_amount1 > 0) IERC20Metadata(lpToken1).safeTransfer(vault, _amount1);
// After we take what is needed we add it all back to our positions.
if (!paused()) _addLiquidity();
(uint256 bal0, uint256 bal1) = balances();
// TVL Balances after withdraw
emit Withdraw(bal0, bal1);
}Impact
This issue can lead to unexpected slippage for the users as well as potential losses for the protocol as a whole.
Recommendations
We recommend ensuring that the onlyCalmPeriods modifier is used in all the external/public functions that can be called by anyone or by the vault. This will prevent users from interacting with the system when it is not safe to do so.
function harvest()
external
+ onlyCalmPeriods
{
_harvest(tx.origin);
}
function withdraw(uint256 _amount0, uint256 _amount1)
external
+ onlyCalmPeriods
{
_onlyVault();
// Liquidity has already been removed in beforeAction() so this is just a simple withdraw.
if (_amount0 > 0) IERC20Metadata(lpToken0).safeTransfer(vault, _amount0);
if (_amount1 > 0) IERC20Metadata(lpToken1).safeTransfer(vault, _amount1);
// After we take what is needed we add it all back to our positions.
if (!paused()) _addLiquidity();
(uint256 bal0, uint256 bal1) = balances();
// TVL Balances after withdraw
emit Withdraw(bal0, bal1);
}Remediation
This issue has been acknowledged by Beefy, and fixes were implemented in the following commits: