Alchemix Transmuter
Alchemix
DeFiFoundrySolidity
16,653 OP
View results
Previous Next
Submission Details
Severity: medium
Valid

Missing claimable amount in _totalAssets calculation during _harvestAndReport which will return an invalid amount of new total asset and make a loss instead of profits breaking the whole strategy.

touthang

Summary

When a profit/loss is realised through the TokenizedStrategy::report() we implement the _harvestAndReport for accurate acounting of new totalAssets i.e. if the new totalAssets is more than the old totalAssets we have a profit and vice versa.

uint256 newTotalAssets = IBaseStrategy(address(this))
.harvestAndReport();
​
uint256 oldTotalAssets = _totalAssets(S);
​
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;
}
.......
........
} else {
// Expect we have a loss.
unchecked {
loss = oldTotalAssets - newTotalAssets;
}
...........
...........

As we can see from the report function in TokenizedStrategy contract, we use harvestAndReport to calculate any profits/loss.

Vulnerability Details

The problem is our implementation of the internal _harvestAndReport function is inacurate and reports an invalid amount of newTotalAssets.

Lets look for the StrategyOp::_harvestAndReport :

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;
}

As we can see, the function falis to add claimable amount in the _totalAssets calculation.

The _totalAssets adds unexchanged balance but not claimable which means it will report a loss breaking the strategy and making a loss for the users.

Example:

  • users deposit 1000 alETH, old totalAsset = 1000

  • and say we make a 2x profit,

  • and currently, unexchanged = 1200 alETH, exchanged/claimable = 800 WETH

  • now, expected new totalAssets = 1200 + 800 = 2000

  • reported new totalAssets = 1200 only

Note: the comment;

// NOTE : possible some dormant WETH that isn't swapped yet
uint256 underlyingBalance = underlying.balanceOf(address(this));

should not be mistaken for the claimable/exchanged tokens because the claimable tokens are held by the transmuter contract.

Also note that the report function is restricted to only the Keeper role but it doesn't matter and will happen regardless.

This issue can also be seen in StrategyMainnet and StrategyArb contracts.

Impact

Profit accounting is broken and users are at loss when they should be profiting.

Tools Used

manual

Recommendations

- _totalAssets = unexchanged + asset.balanceOf(address(this)) + underlyingBalance;
+ _totalAssets = unexchanged + asset.balanceOf(address(this)) + underlyingBalance + claimable;

For the StrategyMainnet and StrategyArb, we can also mitigate it by removing the comments, but still we need to make some adjustments which would be more complex than the provided solution above and introduce another issue. So, the given mitigation should be enough for all the contracts.

Updates

Appeal created

inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Incorrect accounting in `_harvestAndReport` claimable should be included

Support

FAQs

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