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

Total asset tracked in the harvest and report function will return the wrong value of AlETH present

bigsam

Summary

The base strategy calculates the profit/loss of users and since AlETH will always be swapped at a premium price they should always record profit for their users but this is tracked wrongly and AlETH that are claimable are not added/swapped back to AlEth thus Claimable WETH are excluded are a wrong asset value is declared.

This exist int the Op, Mainnet and ARB strategies.

Vulnerability Details

Wrong total asset will be returned.

  1. AlETH and WETH exist in the transmitter.

  2. AlETH and WETH (dusts) may exist in the Base strategy.

/**
* @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)
{
​
@audit>> checked not used>> 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));
​
@audit>> wrong asset value>> _totalAssets = unexchanged + asset.balanceOf(address(this)) + underlyingBalance;
}

We report the AlETH in the transmitter But we fails to include the Claimable (WETH) in the transmitter hence the wrong total value will always be less than the actual token asset and the report will return a loss, impacting the asset to shares ratio and enabling new users to mint more shares than they should and also old depositors loses some asset.

The yearn Vault has a report function that the keeper calls to calculate and report profit/loss

https://github.com/yearn/tokenized-strategy/blob/9ef68041bd034353d39941e487499d111c3d3901/src/TokenizedStrategy.sol#L1055-L1256

/**
* @notice Function for keepers to call to harvest and record all
* profits accrued.
*
* @dev This will account for any gains/losses since the last report
* and charge fees accordingly.
*
* Any profit over the fees charged will be immediately locked
* so there is no change in PricePerShare. Then slowly unlocked
* over the `maxProfitUnlockTime` each second based on the
* calculated `profitUnlockingRate`.
*
* In case of a loss it will first attempt to offset the loss
* with any remaining locked shares from the last report in
* order to reduce any negative impact to PPS.
*
* Will then recalculate the new time to unlock profits over and the
* rate based on a weighted average of any remaining time from the
* last report and the new amount of shares to be locked.
*
* @return profit The notional amount of gain if any since the last
* report in terms of `asset`.
* @return loss The notional amount of loss if any since the last
* report in terms of `asset`.
*/
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.
@audit>> wrong AlETH value returned>> uint256 newTotalAssets = IBaseStrategy(address(this))
.harvestAndReport();
​
@audit>> actual AlETH deposited>>> 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;
​
@audit>>>>> // Calculate profit/loss.
@audit>> wrong value triggers wrong calculations >>> if (newTotalAssets > oldTotalAssets) {
// We have a profit.
unchecked {
@audit>>>>> 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);
​
// Cache the performance fee.
uint16 fee = S.performanceFee;
uint256 totalFeeShares;
// If we are charging a performance fee
if (fee != 0) {
// Asses performance fees.
unchecked {
// Get in `asset` for the event.
@audit>>>>> totalFees = (profit * fee) / MAX_BPS;
// And in shares for the payment.
@audit>>>>> totalFeeShares = (sharesToLock * fee) / MAX_BPS;
}
​
// Get the protocol fee config from the factory.
(
uint16 protocolFeeBps,
address protocolFeesRecipient
) = IFactory(FACTORY).protocol_fee_config();
​
uint256 protocolFeeShares;
// Check if there is a protocol fee to charge.
if (protocolFeeBps != 0) {
unchecked {
// Calculate protocol fees based on the performance Fees.
@audit>>>>> protocolFeeShares =
(totalFeeShares * protocolFeeBps) /
MAX_BPS;
// Need amount in underlying for event.
@audit>>>>> protocolFees = (totalFees * protocolFeeBps) / MAX_BPS;
}
​
// Mint the protocol fees to the recipient.
@audit>>>>> _mint(S, protocolFeesRecipient, protocolFeeShares);
}
​
// Mint the difference to the strategy fee recipient.
unchecked {
_mint(
S,
S.performanceFeeRecipient,
totalFeeShares - protocolFeeShares
);
}
}
​
// Check if we are locking profit.
if (_profitMaxUnlockTime != 0) {
// lock (profit - fees)
unchecked {
sharesToLock -= totalFeeShares;
}
​
// If we are burning more than re-locking.
if (sharesToBurn > sharesToLock) {
// Burn the difference
unchecked {
_burn(S, address(this), sharesToBurn - sharesToLock);
}
} else if (sharesToLock > sharesToBurn) {
// Mint the shares to lock the strategy.
unchecked {
_mint(S, address(this), sharesToLock - sharesToBurn);
}
}
}
} else {
// Expect we have a loss.
unchecked {
@audit>>>>> loss = oldTotalAssets - newTotalAssets;
}
​
// Check in case `else` was due to being equal.
if (loss != 0) {
// We will try and burn the unlocked shares and as much from any
// pending profit still unlocking to offset the loss to prevent any PPS decline post report.
@audit>>>>> sharesToBurn = Math.min(
// Cannot burn more than we have.
S.balances[address(this)],
// Try and burn both the shares already unlocked and the amount for the loss.
_convertToShares(S, loss, Math.Rounding.Down) + sharesToBurn
);
}
​
// Check if there is anything to burn.
@audit>>>>> if (sharesToBurn != 0) {
_burn(S, address(this), sharesToBurn);
}
}
​
// Update unlocking rate and time to fully unlocked.
@audit>>>>> uint256 totalLockedShares = S.balances[address(this)];
if (totalLockedShares != 0) {
uint256 previouslyLockedTime;
uint96 _fullProfitUnlockDate = S.fullProfitUnlockDate;
// Check if we need to account for shares still unlocking.
if (_fullProfitUnlockDate > block.timestamp) {
unchecked {
// There will only be previously locked shares if time remains.
// We calculate this here since it should be rare.
previouslyLockedTime =
(_fullProfitUnlockDate - block.timestamp) *
(totalLockedShares - sharesToLock);
}
}
​
// newProfitLockingPeriod is a weighted average between the remaining
// time of the previously locked shares and the profitMaxUnlockTime.
uint256 newProfitLockingPeriod = (previouslyLockedTime +
sharesToLock *
_profitMaxUnlockTime) / totalLockedShares;
​
// Calculate how many shares unlock per second.
S.profitUnlockingRate =
(totalLockedShares * MAX_BPS_EXTENDED) /
newProfitLockingPeriod;
​
// Calculate how long until the full amount of shares is unlocked.
S.fullProfitUnlockDate = uint96(
block.timestamp + newProfitLockingPeriod
);
} else {
// Only setting this to 0 will turn in the desired effect,
// no need to update profitUnlockingRate.
S.fullProfitUnlockDate = 0;
}
​
// Update the new total assets value.
@audit>>>>> S.totalAssets = newTotalAssets;
@audit>>>>> S.lastReport = uint96(block.timestamp);
​
// Emit event with info
emit Reported(
profit,
loss,
protocolFees, // Protocol fees
totalFees - protocolFees // Performance Fees
);
}

Impact

The Base strategy will always return a loss intent of the actual profit made by the strategy this will impact the asset to shares calculation and the old users who withdraws before this is corrected will loss some assets and new users also will be minted more shares since the total asset has been affected.

According to the yearn v3 documentation

_harvestAndReport()​

Purpose:

  • Called during every report. This should harvest and sell any rewards, reinvest any proceeds, perform any position maintenance and return a full accounting of a trusted amount denominated in the underlying asset the strategy holds.

** Parameters**: NONE

** Returns**:

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

Good to Know:

  • This can only be called by a permissioned address so if set up correctly, it can be trusted to perform swaps, LP movements etc.

  • This should account for both deployed and idle assets.

Best Practice:

  • The returned value is used to account for all strategy profits, losses and fees so care should be taken when relying on oracle values, LP prices etc. that have the potential to be manipulated.

  • This can still be called after a strategy has been shut down so you may want to check if the strategy is shut down before performing certain functions like re-deploying loose funds.

https://docs.yearn.fi/developers/v3/strategy_writing_guide#_deployfundsuint256_amount:~:text=function _harvestAndReport(,)%3B }

Tools Used

Manual Review

Recommendations

Include the claimable amount in the total AlETH calculation or better still swap and then report as done before commenting out the process.

Updates

Appeal created

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

Incorrect accounting in `_harvestAndReport` claimable should be included

balanceDeployed() and _harvestAndReport() add WETH and alETH, but they have different prices

Support

FAQs

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