DeFiFoundrySolidity
16,653 OP
View results
Submission Details
Severity: medium
Valid

Incorrect value returned by `StrategyArb::_harvestAndReport` leads to loss of funds for the user

Summary

StrategyArb::_harvestAndReport is a function that should return an accurate and trusted total amount of 'ASSET' the strategy currently holds, including loose funds. However, the issue is that the function also includes the WETH balance, which cannot be claimed by users. This results in inaccurate asset and share value calculations within the vault, leading to a loss of funds.

Attacker can also use this vulnerability to grief users and make them lose their assets.

Vulnerability Details

https://github.com/Cyfrin/2024-12-alchemix/blob/82798f4891e41959eef866bd1d4cb44fc1e26439/src/StrategyArb.sol#L148-L171

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

From the docs:

_totalAssets: A trusted and accurate account for the total amount of 'asset' the strategy currently holds including loose funds.

As seen in the current implementation of _harvestAndReport, the unexchanged variable is added to _totalAssets. The unexchanged variable represents the current balance of the WETH token within the strategy. However, WETH is the underlying token, not an asset, and cannot be withdrawn by users. Including it in the _totalAssets calculation leads to incorrect asset and share price calculations within the vault.

The WETH can be directly transferred to the strategy address, which may lead to unexpected share and asset prices, potentially opening the door to more serious exploits.

This is an edge case that arises when the keeper is configured to call report regularly, even when no deposits are made and no shares are minted. An attacker could exploit this situation, as WETH is not considered an asset.

  1. The keeper calls report function.

  2. The attacker front-runs it and sends 1 wei of WETH to strategy address.

  3. _totalAssets is updated with current value = 1.

  4. User deposits alETH into vault and exploits it using very well know "donation attack".

A second, more likely scenario involves an attacker transferring any amount of WETH into the vault at any time. This can cause users to receive more alETH than they should when withdrawing, effectively allowing to steal from the last user in the pool. This is a basic griefing attack that results in a loss of funds for the affected user. The direct transfer of WETH disrupts the vault’s share and asset price calculations.

The attacker can:

  1. Deposit into the contract.

  2. Directly send WETH.

  3. Wait for a user deposit.

  4. Withdraw the full amount (attackers alETH + the user's alETH equal to the amount of WETH sent by the attacker).

  5. As a result the attacker lost nothing and the user can't withdraw the full deposited amount (skipping fees of course).

  6. User can only withdraw the remaining alETH available in the strategy - WETH sent by attacker. The attacker could withdraw full deposited amount.

Impact

Adding balance of WETH into _totalAsstes calculation leads to incorrect prices for users. It can be exploited by an attacker to perform a griefing attack.

It is important to note that this report specifically addresses the vulnerability in the StrategyArb contract.

Tools Used

Manual review

Recommended Mitigation

Remove unexchanged from _totalAssets calculation.

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;
+ _totalAssets = asset.balanceOf(address(this)) + underlyingBalance;
}
Updates

Lead Judging Commences

inallhonesty Lead Judge
10 months ago

Appeal created

inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
ke1caM 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.