Summary
The code contains a logical bug in the _harvestAndReport() function, specifically related to how claimable balances are handled. The claimable variable, which represents the potential WETH that can be retrieved from the transmuter, is currently not included in the cumulative total of assets calculated as _totalAssets. This will lead to potential profit loss.
Vulnerability Details
As per the doc of [alchemix](https://docs.alchemix.fi/alchemix-ecosystem/transmuter), the Transmuter is the primary mechanism for restoring alAsset prices and reducing supply by converting alAssets into their underlying assets. Users that deposit alAssets to the Transmuter will gradually be credited with the corresponding assets proportional to the amount of alAssets that they have deposited in the Transmuter. The alAssets are burned when a user claims the transmuted token. That means the returned value of transmuter.getClaimableBalance will gradually increase and the returned value of transmuter.getUnexchangedBalance will gradually decrease. Here is a POC to show this action:
Consider adding an test function in Operation.t.sol
function test_getUnexchangedBalance(uint256 _amount) public{
vm.assume(_amount > minFuzzAmount && _amount < maxFuzzAmount);
mintAndDepositIntoStrategy(strategy, user, _amount);
assertApproxEq(strategy.totalAssets(), _amount, _amount / 500);
vm.roll(1);
deployMockYieldToken();
addMockYieldToken();
depositToAlchemist(_amount);
airdropToMockYield(_amount / 2);
vm.prank(whale);
asset.transfer(user2, _amount);
vm.prank(user2);
asset.approve(address(transmuter), _amount);
vm.prank(user2);
transmuter.deposit(_amount /2 , user2);
console.log("Unexchanged Balance 1:", transmuter.getUnexchangedBalance(address(strategy)));
vm.roll(1);
harvestMockYield();
vm.prank(address(transmuterKeeper));
transmuterBuffer.exchange(address(underlying));
console.log("Unexchanged Balance 2:", transmuter.getUnexchangedBalance(address(strategy)));
assertEq(strategy.totalAssets(), strategy.claimableBalance(), "Force Failure to show log");
}
The logs show as below:
Unexchanged Balance 1: 748717056508893793
Unexchanged Balance 2: 748176616591844954
We can see that the unexchanged balance will gradually decrease.
Let's back to the _harvestAndReport() 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.
*/
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;
}
We can see that the claimable variable, which represents the potential WETH that can be retrieved from the transmuter, is currently not included in the cumulative total of assets calculated as _totalAssets.
As per the comment, the _harvestAndReport() function should harvest all rewards, redeploy any idle funds and return an accurate accounting of all funds currently held by the Strategy. However, the current implementation of the _harvestAndReport() function ignores the claimable variable. This will result in potential profit loss when user withdraw their asset after calling report() . Here is the Poc:
function test_claim_and_swap(uint256 _amount) public {
vm.assume(_amount > minFuzzAmount && _amount < maxFuzzAmount);
mintAndDepositIntoStrategy(strategy, user, _amount);
console.log("user deposit amount:", _amount);
assertApproxEq(strategy.totalAssets(), _amount, _amount / 500);
vm.roll(1);
deployMockYieldToken();
addMockYieldToken();
depositToAlchemist(_amount);
airdropToMockYield(_amount / 2);
airdrop(underlying, address(transmuterKeeper), _amount);
vm.prank(transmuterKeeper);
underlying.approve(address(transmuterBuffer), _amount);
*/
vm.prank(whale);
asset.transfer(user2, _amount);
vm.prank(user2);
asset.approve(address(transmuter), _amount);
vm.prank(user2);
transmuter.deposit(_amount /2 , user2);
vm.roll(1);
harvestMockYield();
vm.prank(address(transmuterKeeper));
transmuterBuffer.exchange(address(underlying));
skip(7 days);
vm.roll(5);
vm.prank(user2);
transmuter.deposit(_amount /2 , user2);
vm.prank(address(transmuterKeeper));
transmuterBuffer.exchange(address(underlying));
assertGt(strategy.claimableBalance(), 0, "!claimableBalance");
assertEq(strategy.totalAssets(), _amount);
uint256 claimable = strategy.claimableBalance();
skip(1 seconds);
vm.roll(1);
vm.prank(keeper);
(uint256 profit, uint256 loss) = strategy.report();
console.log("report Profit: ", profit);
console.log("report loss: ", loss);
vm.prank(keeper);
if (block.chainid == 1) {
console.log("Mainnet");
IStrategyInterface(address(strategy)).claimAndSwap(
claimable,
claimable * 101 / 100,
0
);
} else if (block.chainid == 10) {
console.log("op");
IVeloRouter.route[] memory veloRoute = new IVeloRouter.route[]();
veloRoute[0] = IVeloRouter.route(address(underlying), address(asset), true, 0xF1046053aa5682b4F9a81b5481394DA16BE5FF5a);
IStrategyInterfaceVelo(address(strategy)).claimAndSwap(claimable, claimable * 101 / 100, veloRoute);
} else if (block.chainid == 42161) {
console.log("arb");
IRamsesRouter.route[] memory ramsesRoute = new IRamsesRouter.route[]();
address eFrax = 0x178412e79c25968a32e89b11f63B33F733770c2A;
ramsesRoute[0] = IRamsesRouter.route(address(underlying), eFrax, true);
ramsesRoute[1] = IRamsesRouter.route(eFrax, address(asset), true);
IStrategyInterfaceRamses(address(strategy)).claimAndSwap(claimable, claimable * 103 / 100, ramsesRoute);
} else {
revert("Chain ID not supported");
}
uint256 shares = strategy.balanceOf(user);
console.log("user withdraw shares:",shares);
vm.prank(user);
strategy.approve(address(strategy),shares);
vm.prank(user);
uint256 assetReturned = strategy.redeem(shares,user,user);
console.log("user asset get back:",assetReturned);
console.log("user asset loss:",_amount - assetReturned);
vm.prank(keeper);
(profit, loss) = strategy.report();
console.log("report Profit: ", profit);
console.log("report loss: ", loss);
vm.prank(keeper);
assertEq(strategy.claimableBalance(), 0, "!claimableBalance");
assertGt(strategy.totalAssets(), _amount, "!totalAssets");
assertEq(strategy.totalAssets(), strategy.claimableBalance(), "Force Failure");
}
The test_claim_and_swap will first call report() , and then the user calls redeem to withdraw all funds, the result is:
Logs:
user deposit amount: 748717056508893793
report Profit: 0
report loss: 540439917048839
Mainnet
user withdraw shares: 748717056508893793
user asset get back: 748176616591844954
user asset loss: 540439917048839
report Profit: 547705996131862
report loss: 0
The first report() will result in some loss because at this time, the unexchanged balance has decreased but the claimable is not added to the _totalAssets. If the user withdraw all funds after the first report, the user will bear this loss.
Let's add the claimable to the _totalAssets (The code here is only for test, the more reasonable way is to harvest all rewards, redeploy any idle funds and return an accurate accounting of all funds currently held by the Strategy)
_totalAssets = claimable + unexchanged + asset.balanceOf(address(this)) + underlyingBalance;
and run the test again, the result is:
Logs:
user deposit amount: 748717056508893793
report Profit: 0
report loss: 0
Mainnet
user withdraw shares: 748717056508893793
user asset get back: 748717056508893793
user asset loss: 0
report Profit: 7266079083023
report loss: 0
We can see that there is no loss and the user can withdraw all his funds.
Impact
The user may suffer a loss in some case, the impact is High and the likelihood is Low, so the severity should be medium
Tools Used
Manual Review
Recommendations
Consider harvesting all rewards in _harvestAndReport() function and returning an accurate accounting of all funds currently held by the Strategy.
