Alchemix Transmuter
Alchemix
DeFiFoundrySolidity
16,653 OP
View results
Previous Next
Submission Details
Severity: high
Invalid

Incorrect `_harvestAndReport` function implementation

pontifex

Summary

The _harvestAndReport function should return the new value of the strategy totalAssets which reflects the strategy profit. But the current implementation of the function might return stale information about actual asset balances which cause incorrect shares locking periods. This causes incorrect shares/assets ratio when depositing and withdrawing. The issue appears in all strategies of the scope.

Vulnerability Details

The _harvestAndReport function is an important part of the strategy logic, which calculates how many additional shares should be minted and locked for slow rewards unlocking. But the current implementation just returns the assets current balances which might be updated a long before.

function _harvestAndReport() //@audit does not check if TokenizedStrategy.isShutdown but this function is view in this condition
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 (although we can restrict to only claim & swap in one tx)
uint256 underlyingBalance = underlying.balanceOf(address(this));
_totalAssets = unexchanged + asset.balanceOf(address(this)) + underlyingBalance;function
}

The report function relays on the harvestAndReport to calculate shares:

function report()
external
nonReentrant
onlyKeepers
returns (uint256 profit, uint256 loss)
{
// Cache storage pointer since its used repeatedly.
StrategyData storage S = _strategyStorage();
​
// Tell the strategy to report the real total assets it has.
// It should do all reward selling and redepositing now and
// account for deployed and loose `asset` so we can accurately
// account for all funds including those potentially airdropped
// and then have any profits immediately locked.
>> uint256 newTotalAssets = IBaseStrategy(address(this))
.harvestAndReport();
​
uint256 oldTotalAssets = _totalAssets(S);
​
// Get the amount of shares we need to burn from previous reports.
uint256 sharesToBurn = _unlockedShares(S);
​
// Initialize variables needed throughout.
uint256 totalFees;
uint256 protocolFees;
uint256 sharesToLock;
uint256 _profitMaxUnlockTime = S.profitMaxUnlockTime;
// Calculate profit/loss.
if (newTotalAssets > oldTotalAssets) {
// We have a profit.
unchecked {
>> profit = newTotalAssets - oldTotalAssets;
}
​
// We need to get the equivalent amount of shares
// at the current PPS before any minting or burning.
>> sharesToLock = _convertToShares(S, profit, Math.Rounding.Down);

Impact

Unintended behavior, potential asset losses, incorrect shares/assets conversion

Tools used

Manual Review

Recommendations

Consider calling claimAndSwap at the same time with the report function or including claimAndSwap call as internal in the _harvestAndReport function.

Updates

Appeal created

inallhonesty Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
pontifex Submitter
7 months ago
inallhonesty Lead Judge
7 months ago
inallhonesty Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.