Summary
One of the purpose of _harvestAndReport to account total amount of assets the strategy currently holds including idle funds as we can see from the Nat spec of the function:
* @dev Internal function to harvest all rewards, redeploy any idle
* funds and return an accurate accounting of all funds currently
* held by the Strategy.
*
* This should do any needed harvesting, rewards selling, accrual,
* redepositing etc. to get the most accurate view of current assets.
*
* NOTE: All applicable assets including loose assets should be
* accounted for in this function.
*
* Care should be taken when relying on oracles or swap values rather
* than actual amounts as all Strategy profit/loss accounting will
* be done based on this returned value.
*
* This can still be called post a shutdown, a strategist can check
* `TokenizedStrategy.isShutdown()` to decide if funds should be
* redeployed or simply realize any profits/losses.
*
* @return _totalAssets A trusted and accurate account for the total
* amount of 'asset' the strategy currently holds including idle funds.
*/
The current design of _harvestAndReport looks like this:
In StrategyMainnet & StrategyArb it looks like this:
function _harvestAndReport()
internal
override
returns (uint256 _totalAssets)
{
uint256 claimable = transmuter.getClaimableBalance(address(this));
if (claimable > 0) {
}
uint256 unexchanged = transmuter.getUnexchangedBalance(address(this));
uint256 underlyingBalance = underlying.balanceOf(address(this));
_totalAssets = unexchanged + asset.balanceOf(address(this)) + underlyingBalance;
}
In StrategyOp it looks like this:
function _harvestAndReport()
internal
override
returns (uint256 _totalAssets)
{
uint256 claimable = transmuter.getClaimableBalance(address(this));
uint256 unexchanged = transmuter.getUnexchangedBalance(address(this));
uint256 underlyingBalance = underlying.balanceOf(address(this));
_totalAssets = unexchanged + asset.balanceOf(address(this)) + underlyingBalance;
}
Vulnerability Details
As we can see the common issue is with this line:
_totalAssets = unexchanged + asset.balanceOf(address(this)) + underlyingBalance;
It return the wrong amount of _totalAssets since it does not include claimable amount into calculation.
Impact
This can give protocol wrong return data in report and could cause issue in further implementation especially where the accurate amount of _totalAssets is important for accounting purposes (like in report calculation of profit/loss for a strategy in TokenizedStrategy and FlatTokenizedStrategy).
Tools Used
Manual Review
Recommendations
Make the following changes in _harvestAndReport:
In StrategyMainnet and StrategyArb:
function _harvestAndReport()
internal
override
returns (uint256 _totalAssets)
{
uint256 claimable = transmuter.getClaimableBalance(address(this));
if (claimable > 0) {
// transmuter.claim(claimable, address(this));
}
// NOTE : we can do this in harvest or can do seperately in tend
// if (underlying.balanceOf(address(this)) > 0) {
// _swapUnderlyingToAsset(underlying.balanceOf(address(this)));
// }
uint256 unexchanged = transmuter.getUnexchangedBalance(address(this));
// NOTE : possible some dormant WETH that isn't swapped yet
uint256 underlyingBalance = underlying.balanceOf(address(this));
+ _totalAssets = unexchanged + asset.balanceOf(address(this)) + underlyingBalance + claimable;
}
In StrategyOp :
function _harvestAndReport()
internal
override
returns (uint256 _totalAssets)
{
uint256 claimable = transmuter.getClaimableBalance(address(this));
uint256 unexchanged = transmuter.getUnexchangedBalance(address(this));
// NOTE : possible some dormant WETH that isn't swapped yet
uint256 underlyingBalance = underlying.balanceOf(address(this));
+ _totalAssets = unexchanged + asset.balanceOf(address(this)) + underlyingBalance + claimable;
}
