Therefore, once the condition lastHarvest + harvestCooldown < block.timestamp is true, it will remain true (unless the AdapterBase is reinitialized which won't happen).
Making harvestCooldown useless.
Once block.timestamp > lastHarvest + harvestCooldown, where harvestCooldown < 1 days, anyone calling one of these four functions will trigger strategy.harvest() which will result in an unexpected waste of gas (as big as the strategy needs).
Depending on what the strategy does, modifying the vault/adapter's token balances could lead to an unexpected loss of tokens. (referring to the README.md::Grieving Attacks list)
Moreover, function harvest() public takeFees is public, with no access control, which means that anyone can call it at any time.
Depending on the strategy and the implementation of strategy.harvest(), an attacker could exploit the fact that harvest() can be called arbitrarily. (e.g strategy includes a UniswapV2 Pool which is a constant-product AMM: it could be subject to a price manipulation attack (powered by a flashloan) in favor of the attacker).
This attack surface is amplified by the fact that the delegated call to strategy.harvest() can silently fail.
Proof of Concept
I used the BeefyAdapter for this PoC, but it would work the same way with any adapter that inherits AdapterBase.
The strategy used is the MockStrategy.sol, its harvest function emits the event "StrategyExecuted()" when called.
The PoC relies on checking if the current timestamp is in the expected interval and on the expected emission of StrategyExecuted().
uint256 public lastHarvest from AdapterBase.sol has no getter function in IAdapter.sol, which is why initialLastHarvest & updatedLastHarvest are used to assert the interval in the test test__harvest_cooldown().
The test set harvestCooldown as 1 hour (3600 seconds)
adapter.harvest() is then called at different times:
When block.timestamp < lastHarvest + harvestCooldown, which works as intended, emitting Harvested() but not StrategyExecuted()
When lastHarvest + harvestCooldown < block.timestamp, which works as intended, emitting Harvested() and StrategyExecuted().
When updatedLastHarvest + harvestCooldown > block.timestampupdatedLastHarvest is the value that should've taken lastHarvest on the past call of adapter.harvest(). We shouldn't have an emission of StrategyExecuted() but we do when checking the logs:
Tools Used
Manual review
Recommended Mitigation Steps
Update the lastHarvest sotrage variable to block.timestamp in the harvest() function (following the Check-Effect-Interaction pattern):
/**
* @notice Execute Strategy and take fees. Updates lastHarvest
* @dev Delegatecall to strategy's harvest() function. All necessary data is passed via `strategyConfig`.
* @dev Delegatecall is used to in case any logic requires the adapters address as a msg.sender. (e.g. Synthetix staking)
*/
function harvest() public takeFees {
if (
address(strategy) != address(0) &&
((lastHarvest + harvestCooldown) < block.timestamp)
) {
lastHarvest = block.timestamp;
// solhint-disable
//Delegated call no return value should also be done
address(strategy).delegatecall(
abi.encodeWithSignature("harvest()")
);
}
emit Harvested();
}
Lines of code
https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L433-L450
Vulnerability details
Impact
The variable
uint256 public lastHarvest`` is initialized by
__AdapterBaseinitto
block.timestamp` but it is never updated afterward. [Initialisation of lastHarvest in \_AdapterBase_init() GitHub Link](https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/adapter/abstracts/AdapterBase.sol#L42-L87)Therefore, once the condition
lastHarvest + harvestCooldown < block.timestamp
is true, it will remain true (unless the AdapterBase is reinitialized which won't happen). MakingharvestCooldown
useless.The
harvest
function is used in:_deposit(address caller, address receiver, uint256 assets, uint256 shares)
_deposit function in AdapterBase.sol GitHub Link_withdraw(address caller, address receiver, address owner, uint256 assets, uint256 shares)
_withdraw function in AdapterBase.sol GitHub LinkThese two internal functions are used by these four external functions:
deposit(uint256 assets, address receiver)
: deposit function in AdapterBase.sol GitHub Linkmint(uint256 shares, address receiver)
: mint function in AdapterBase.sol GitHub Linkwithdraw(uint256 assets, address receiver, address owner)
: withdraw function in AdapterBase.sol GitHub Linkredeem(uint256 shares, address receiver, address owner)
: redeem function in AdapterBase.sol GitHub LinkOnce
block.timestamp > lastHarvest + harvestCooldown
, whereharvestCooldown < 1 days
, anyone calling one of these four functions will triggerstrategy.harvest()
which will result in an unexpected waste of gas (as big as the strategy needs). Depending on what the strategy does, modifying the vault/adapter's token balances could lead to an unexpected loss of tokens. (referring to the README.md::Grieving Attacks list)Moreover,
function harvest() public takeFees
is public, with no access control, which means that anyone can call it at any time.Depending on the strategy and the implementation of
strategy.harvest()
, an attacker could exploit the fact thatharvest()
can be called arbitrarily. (e.g strategy includes a UniswapV2 Pool which is a constant-product AMM: it could be subject to a price manipulation attack (powered by a flashloan) in favor of the attacker).This attack surface is amplified by the fact that the delegated call to
strategy.harvest()
can silently fail.Proof of Concept
I used the BeefyAdapter for this PoC, but it would work the same way with any adapter that inherits AdapterBase.
harvest
function emits the event "StrategyExecuted()" when called.uint256 public lastHarvest
from AdapterBase.sol has no getter function in IAdapter.sol, which is whyinitialLastHarvest
&updatedLastHarvest
are used to assert the interval in the testtest__harvest_cooldown()
.adapter.harvest()
is then called at different times:block.timestamp < lastHarvest + harvestCooldown
, which works as intended, emitting Harvested() but not StrategyExecuted()lastHarvest + harvestCooldown < block.timestamp
, which works as intended, emitting Harvested() and StrategyExecuted().updatedLastHarvest + harvestCooldown > block.timestamp
updatedLastHarvest
is the value that should've takenlastHarvest
on the past call ofadapter.harvest()
. We shouldn't have an emission of StrategyExecuted() but we do when checking the logs:Tools Used
Manual review
Recommended Mitigation Steps
Update the lastHarvest sotrage variable to block.timestamp in the harvest() function (following the Check-Effect-Interaction pattern):