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

_harvestAndReport returns a wrong calculation of gains

strapontin

Summary

As per the Yearn's documentation and the comments in the code, _harvestAndReport should return "A trusted and accurate account for the total amount of 'asset' the strategy currently holds including loose funds", but for all 3 strategies (mainnet, Op and Arb), the function returns the sum of all unexchanged tokens from the transmuter, the balance of asset for the strategy, and the balance of underlying for the strategy, corrupting the shares given to future depositors.

Vulnerability Details

Code snippet from StrategyOp.sol
Note : I chose to pick the code from this strategy as it was the cleanest, but the other 2 strategies fundamentally implement the same logic.

function _harvestAndReport()
internal
override
returns (uint256 _totalAssets)
{
// @audit : `claimable` variable is unused
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;
}

_harvestAndReport is used when the keeper calls the report function to generate profit for vault's token holders. The profit calculation depends of the result value of _harvestAndReport (calculated at the beginning of the report function).

The code doesn't return only "'asset' the strategy currently holds", because while unexchanged + asset.balanceOf(address(this)) are the sum of withdrawable funds (alETH) in the transmuter + idle funds, underlyingBalance is the amount of idle WETH in the strategy, and the strategy withdrawable asset is only alETH.

If a user sends WETH to the strategy maliciously or by mistake, the accounting of profit will become wrong, because no WETH is ever withdrawable from the strategy.

Since the profit slowly unlocks over the maxProfitUnlockTime, a user who deposits straight after a report won't be impacted, but a user who deposits after the WETH is entirely accounted for profit will suffer the most disadvantage, as he receives less shares than other depositors for the same amount of alETH deposited, and will earn less yield for the future profits.

Also, while it isn't the root cause of the issue, the fact that WETH can't be removed from the strategy makes the issue worse than it could be, since the keeper can't withdraw the WETH before calling report, which would have been a possible mitigation of the problem.

Impact

Loss of yield profit for several users. Unfair advantage to users who deposit before the issue occurs.

PoC

Copy the following PoC in Operation.t.sol. Run with forge test --mt test_weth_harvest_report --fork-url $ETH_RPC_URL.

function test_weth_harvest_report() public {
address alice = makeAddr("alice");
address bob = makeAddr("bob");
​
address charlie = makeAddr("charlie");
deal(address(underlying), charlie, 1e18);
​
address postDepositor1 = makeAddr("postDepositor1");
address postDepositor2 = makeAddr("postDepositor2");
​
uint256 _amount = 50e18;
​
// Both Alice & Bob deposit 100 alETH into strategy
mintAndDepositIntoStrategy(strategy, alice, _amount);
mintAndDepositIntoStrategy(strategy, bob, _amount);
assertEq(strategy.totalAssets(), _amount * 2, "!totalAssets");
​
// Charlie accidently or maliciously sends WETH to the strategy
vm.prank(charlie);
ERC20(underlying).transfer(address(strategy), 1e18);
​
// Report profit
vm.prank(keeper);
(uint256 profit,) = strategy.report();
​
// The strategy records a profit
assertEq(profit, 1e18);
​
// Just after the report, the profit isn't unlocked
// This means depositors are not yet affected (see assertions at the end)
mintAndDepositIntoStrategy(strategy, postDepositor1, _amount);
​
// The "profit" is slowly released over time
skip(strategy.profitMaxUnlockTime());
​
// When the false profit is fully released, future depositors will mint
// less shares than they should
mintAndDepositIntoStrategy(strategy, postDepositor2, _amount);
​
uint256 sharesOfAlice = strategy.balanceOf(address(alice));
uint256 sharesOfBob = strategy.balanceOf(address(bob));
uint256 sharesOfPostDepositor1 = strategy.balanceOf(address(postDepositor1));
uint256 sharesOfPostDepositor2 = strategy.balanceOf(address(postDepositor2));
​
// All depositors have the same amount of shares expect for those who deposited
// after the false profit is fully released, even though the alETH balance
// of the contract has not changed.
assertEq(sharesOfAlice, sharesOfBob);
assertEq(sharesOfAlice, sharesOfPostDepositor1);
​
assertGe(sharesOfAlice, sharesOfPostDepositor2);
// Because postDepositor2 has less asset than other depositors,
// he will earn less rewards when real alETH are swapped for profit
}

Recommendations

Have _harvestAndReport return only the sum of alETH available (like the availableWithdrawLimit does currently).

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

Also, adding a method to withdraw tokens in the strategy (except alETH) can be a good idea to avoid having WETH stuck here.

Updates

Appeal created

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

Incorrect accounting in `_harvestAndReport` claimable should be included

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

Dormant WETH is not properly treated

Support

FAQs

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