github-actions[bot] commented 4 months ago

SBSecurity

medium

`OptimismRewardCollector` uses `ETH` oracle but swap to `WETH` assuming the price is 1:1

Summary

OptimismRewardCollector assumes that WETH price vs ETH price is 1:1

Vulnerability Detail

When rewards are distributed through RewardRouter.distributeRewards(), at the end it calls OptimismRewardCollector.claimAndDonateRewards(), and pass the result from OptimismRewardCollector.getExpectedExchange as minimumAmountOut to claimAndDonateRewards().

function distributeRewards(address vault) external returns (uint256) {
    ...

    return IRewardCollector(rewards[vault].rewardCollectorAddress).claimAndDonateRewards(vault, IRewardCollector(rewards[vault].rewardCollectorAddress).getExpectedExchange(vault) * slippageBPS / BPS);
}

// function claimAndDonateRewards(address token, uint256 minimumAmountOut)

OptimismRewardCollector.getExpectedExchange converts from a rewardToken(Optimism) amount to a corresponding USD or ETH value based on the debtToken (can be alUSD or alETH).

function getExpectedExchange(address yieldToken) external view returns (uint256) {
    uint256 expectedExchange;
    address[] memory token = new address[](1);
    uint256 totalToSwap = TokenUtils.safeBalanceOf(rewardToken, address(this));

    // Ensure that round is complete, otherwise price is stale.
    (
        uint80 roundID,
        int256 opToUsd,
        ,
        uint256 updateTime,
        uint80 answeredInRound
    ) = IChainlinkOracle(opToUsdOracle).latestRoundData();

    require(
        opToUsd > 0, 
        "Chainlink Malfunction"
    );

    if( updateTime < block.timestamp - 1200 seconds ) {
        revert("Chainlink Malfunction");
    }

    // Ensure that round is complete, otherwise price is stale.
    (
        uint80 roundIDEth,
        int256 ethToUsd,
        ,
        uint256 updateTimeEth,
        uint80 answeredInRoundEth
    ) = IChainlinkOracle(ethToUsdOracle).latestRoundData();

    require(
        ethToUsd > 0, 
        "Chainlink Malfunction"
    );

    if( updateTimeEth < block.timestamp - 1200 seconds ) {
        revert("Chainlink Malfunction");
    }

    // Find expected amount out before calling harvest
    if (debtToken == alUsdOptimism) {
        expectedExchange = totalToSwap * uint(opToUsd) / 1e8;
    } else if (debtToken == alEthOptimism) {
        expectedExchange = totalToSwap * uint(uint(opToUsd)) / uint(ethToUsd);
    } else {
        revert IllegalState("Invalid debt token");
    }

    return expectedExchange;
}

We will be looking at the alEthOptimism case. It will first convert rewardToken(Optimism) to USD and then divide by ETH, thus finding how much alETH to get for passed totalToSwap.

ethToUsd price is the actual price of ETH taken from here address constant ethToUsdOracle = 0x13e3Ee699D1909E989722E753853AE30b17e08c5;

Then in claimAndDonateRewards() when debtToken is alETH (already deployed at 0x3E29D3A9316dAB217754d13b28646B76607c5f04. It will swap from Optimism 0x4200000000000000000000000000000000000042 to WETH on Optimism 0x4200000000000000000000000000000000000006 and then to alETH 0x3E29D3A9316dAB217754d13b28646B76607c5f04, but as we showed above, it converts the minimumAmountOut that is passed to an ETH value.

function claimAndDonateRewards(address token, uint256 minimumAmountOut) external returns (uint256) {
    require(msg.sender == rewardRouter, "Must be Reward Router"); 

    // Amount of reward token claimed plus any sent to this contract from grants.
    uint256 amountRewardToken = IERC20(rewardToken).balanceOf(address(this));

    if (amountRewardToken == 0) return 0;

    if (debtToken == 0xCB8FA9a76b8e203D8C3797bF438d8FB81Ea3326A) {
    ... USD Case
    } else if (debtToken == 0x3E29D3A9316dAB217754d13b28646B76607c5f04) {
        // Velodrome Swap Routes: OP -> alETH
        IVelodromeSwapRouter.Route[] memory routes = new IVelodromeSwapRouter.Route[](2);
        routes[0] = IVelodromeSwapRouter.Route(0x4200000000000000000000000000000000000042, 0x4200000000000000000000000000000000000006, false, 0xF1046053aa5682b4F9a81b5481394DA16BE5FF5a);
        routes[1] = IVelodromeSwapRouter.Route(0x4200000000000000000000000000000000000006, 0x3E29D3A9316dAB217754d13b28646B76607c5f04, true, 0xF1046053aa5682b4F9a81b5481394DA16BE5FF5a);
        TokenUtils.safeApprove(rewardToken, swapRouter, amountRewardToken);
        IVelodromeSwapRouter(swapRouter).swapExactTokensForTokens(amountRewardToken, minimumAmountOut, routes, address(this), block.timestamp);
    } else {
        revert IllegalState("Reward collector `debtToken` is not supported");
    }

    // Donate to alchemist depositors
    uint256 debtReturned = IERC20(debtToken).balanceOf(address(this));
    TokenUtils.safeApprove(debtToken, alchemist, debtReturned);
    IAlchemistV2(alchemist).donate(token, debtReturned);

    return amountRewardToken;
}

Here you can see the price chart of ETH vs WETH for April 18, 2024 and as I indicated the maximum difference was $41.

If search a bit harder can find an example for almost $100 diff

Untitled

That $41 in getExpectedExchange() will result in 4100000000 added to the number, but then will swap to WETH which was actually at $3019 compared to ETH at $3060.

Example params for 18 April 2024 2:30 PM

totalToSwap (rewardToken in OptimismRewardCollector) = 1000e18
Optimism price = $2.24 (223989858 from Oracle)
ETH price = $3,060.00 (306000000000 from Oracle)
WETH price = $3,019,00 (301900000000 if use Oracle)

// Find expected amount out before calling harvest
if (debtToken == alUsdOptimism) {
    expectedExchange = totalToSwap * uint(opToUsd) / 1e8;
} else if (debtToken == alEthOptimism) {
    expectedExchange = totalToSwap * uint(uint(opToUsd)) / uint(ethToUsd);
} else {
    revert IllegalState("Invalid debt token");
}

The result will be as follow:

If use ETH price
- 1000e18 * 223989858 / 306000000000 = 731993000000000000 (0.731993 alETH)
If use WETH price
- 1000e18 * 223989858 / 301900000000 = 741933945014905597.88………….. (0.741933945014905597880092…….. alETH)

This number will be passed as minimumAmountOut, when perform the swap. In this case above (ETH > WETH), the minimumAmountOut will be lower that the actual, causing the user to lose out on their rewards when the swap takes place. In the other case (WETH > ETH), the minimumAmountOut will be higher that the actual, which may revert (because of slippage) in cases where it is close to it, but need to be lower.

Impact

Using the wrong Oracle to compute the slippage may block the swap or lose user rewards.

Code Snippet

https://github.com/sherlock-audit/2024-04-alchemix/blob/32e4902f77b05ea856cf52617c55c3450507281c/v2-foundry/src/utils/collectors/OptimismRewardCollector.sol#L136

https://github.com/sherlock-audit/2024-04-alchemix/blob/32e4902f77b05ea856cf52617c55c3450507281c/v2-foundry/src/utils/collectors/OptimismRewardCollector.sol#L76-L78

Tool used

Manual Review

Recommendation

Consider using a WETH oracle to be as accurate as possible.

Hesnicewithit commented 4 months ago

WETH oracles isnt a thing because it is always exchangeable for 1:1 ETH

Hash01011122 commented 4 months ago

Thanks for your input wasn't aware of this fact invalidating this issue.

Slavchew commented 4 months ago

@Hash01011122

Here WETH is 1:1 with ETH in amount, yes, but not in price as I showed in the report. Even now, at the time of writing, there is a $10 difference, making the slippage calculation wrong. The issue shows that the slippage calculation is based on ETH and the swap to WETH, very similar to this one https://github.com/sherlock-audit/2024-04-alchemix-judging/issues/5 which snows that the slippage is based on OP/USD and OP/ETH, but then swapped to alUSD and alETH.

AtanasDimulski commented 4 months ago

Escalate, Check @Slavchew above comment

sherlock-admin3 commented 4 months ago

Escalate, Check @Slavchew above comment

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.

Hesnicewithit commented 4 months ago

Coinmarketcap is not a price oracle. People dont buy or trade for WETH they exchange it 1:1 in the weth contract. Look at any DEX and it will say swapping ETH to WETH will be 1:1 since they aren't doing anything other than unwrapping/wrapping. Chainlink does not offer a WETH oracle because it is always 1:1

WangSecurity commented 4 months ago

I believe this should be a duplicate of #5

@Slavchew do you agree or is there something that I miss?

WangSecurity commented 4 months ago

Planning to accept the escalation and duplicate to #5 .

gstoyanovbg commented 4 months ago

I agree with @Hesnicewithit and don't think it is a dup of #5 but rather invalid.

@WangSecurity Can you elaborate why do you think this is a valid issue and duplicate of #5 ?

WangSecurity commented 4 months ago

I believe both this report and report #5 explain the core issue, which is that the protocol uses ETH price when it comes to calculation of alETH and WETH price. I agree with the sponsor that WETH is only a wrapped version of WETH and exchanged 1:1, but when it comes to alETH, it is not exchanged 1:1 and their prices have differences, which are not taken into account. I see that this report is focused on WETH, but it explains that the ending amount of alETH is different from the amount received after OP to ETH swap, and will bypass slippage checks:

Then in claimAndDonateRewards() when debtToken is alETH (already deployed at 0x3E29D3A9316dAB217754d13b28646B76607c5f04. It will swap from Optimism 0x4200000000000000000000000000000000000042 to WETH on Optimism 0x4200000000000000000000000000000000000006 and then to alETH 0x3E29D3A9316dAB217754d13b28646B76607c5f04, but as we showed above, it converts the minimumAmountOut that is passed to an ETH value.

I believe that the impact in this report is also valid, that bypassing these checks, due to wrong exchange price used, will lead to receiving less/more alETH than intended, hence, loss of funds.

Regarding CoinMarketCap, the report doesn't suggest to use CoinMarketCap as an oracle or to define the price, but as an example to show the difference in prices. If you think my assumptions are wrong, will be glad to know where.

But, me decision remains the same, accept the escalation and leave the issue as it is.

gstoyanovbg commented 4 months ago

As far as I understand from this discussion and the comments in Discord, we all agree that ETH and WETH are exchanged at a ratio of 1:1 through the WETH contract. This is logical because the main idea of WETH is for protocols not to have to implement additional logic every time they work with ETH, but instead to use unified logic for all ERC20 tokens. This is also the reason why there is no WETH/ETH oracle in Chainlink - it would be pointless and no one would use it. So why do some websites show a slightly lower price for WETH measured in USD compared to ETH measured in USD? The answer is that there are risks associated with WETH resulting from the implementation of the smart contract, such as bugs in the source code. But that does not mean that WETH and ETH are traded at a ratio other than 1:1 because a significant difference in the ratio would mean that a profitable MEV attack could be made.

Let's imagine that we have 2 pools OP/ETH and OP/WETH assuming that the prices are different. For example:

Pool 1: OP/ETH = 2 Pool 2: OP/WETH = 4

Let a user have 1 OP initially and perform the following sequence of operations:

Initially 1 OP -> Sell 1 OP, Buy 4 WETH from pool 2 -> convert 4 WETH to 4 ETH using the WETH contract -> Sell 4 ETH, Buy 2 OP from pool 1 ... and so on.

You can see that after a series of operations, the user can double their OP tokens using the fact that WETH:ETH is exchanged at a ratio of 1:1. Therefore, there cannot be a significant difference in the price of OP measured in ETH and WETH on-chain.

Returning to the current report, if we assume that ETH and WETH have different prices, then OP/ETH and OP/WETH must also be different. This is not possible, as I proved above. Therefore, what is described in this report is not possible and does not represent a risk.

Slavchew commented 4 months ago

Who said the price is 2x higher and who said it will be transferred this way, the report states that due to a small price difference, the slippage can be bypassed as @WangSecurity mentioned above. The report focuses on the WETH part, yes it's not the best explained, but that's why there is a selected one and duplicates.

@WangSecurity what's your last word because if we keep going like this it could take weeks.

WangSecurity commented 4 months ago

As I've said above, yes the report focuses on WETH. It identifies the core issue which is the protocol using OP/ETH price when converting to other assets. It identifies the impact that the slippage check will bypass leading to users receiving less rewards. It shows the path from OP to alETH, which explains that the end amount of alETH will be incorrect (since using OP/ETH price). Hence, I believe it's a valid duplicate.

Planning to accept the escalation and duplicate with report 5.

Hash01011122 commented 4 months ago

@WangSecurity I think this should be accepted as unique medium rather than dup of #5, though the root cause is same but attack vector is different.

Scenario A: There is a root cause/error/vulnerability A in the code. This vulnerability A -> leads to two attack paths:

B -> high severity path

C -> medium severity attack path/just identifying the vulnerability.

WangSecurity commented 4 months ago

@Hash01011122 in that scenario reports B and C are duplicates, hence, this report has to be duplicated with #5:

Scenario A: There is a root cause/error/vulnerability A in the code. This vulnerability A -> leads to two attack paths:

B -> high severity path

C -> medium severity attack path/just identifying the vulnerability. Both B & C would not have been possible if error A did not exist in the first place. In this case, both B & C should be put together as duplicates.

gstoyanovbg commented 4 months ago

@WangSecurity I would agree with your conclusions if this report showed a valid attack path. But that's not the case. The calculations showing a significant price difference between ETH and WETH in USD (and consequently in alETH) are based on incorrect data. Therefore, all conclusions made thereafter are based on erroneous information, rendering them invalid. It has already been mentioned that CoinMarketCap is not an oracle and is not created to be 100% accurate. It's not coincidental that when we need the price of a crypto asset to perform a certain operation, we don't use a random website but instead use oracles designed to ensure that the price is current at any given moment. Chainlink does not have a WETH/USD feed (I leave it to the readers to figure out why). However, Pyth Network is another oracle that has a WETH/USD feed.

https://pyth.network/price-feeds/crypto-weth-usd https://pyth.network/price-feeds/crypto-eth-usd

At the time of writing this comment, the price of WETH is $3002, and that of ETH is $3003. In percentage terms, the difference is around 0.04%. This is a negligible number, so we cannot talk about any slippage problem arising from it. Most of the time, the difference is even smaller. In my previous comment, I demonstrated why there cannot be a significant difference in the two prices on-chain because it creates arbitrage opportunities. If there is a significant difference somewhere, I would be glad to know where, and I would be the first to make an arbitrage there.

Hash01011122 commented 4 months ago

I have gone through this issue again, as @WangSecurity mentioned at first I thought it can be considered as #5 but the attack path highlighted by Watson is inaccurate.

From Report:

That $41 in getExpectedExchange() will result in 4100000000 added to the number, but then will swap to WETH which was actually at $3019 compared to ETH at $3060.

According to pyth network documentation the confidence(How far from the Aggregate Price, true price might be), The range of the confidence interval provided by Pyth for an asset, say Bitcoin, would be calculated as

μ+σ minus 𝜇−𝜎. If we simplify this expression, it equals 2𝜎.

In the example given where Bitcoin's price is $50000±$10 $50000±$10, the range of the confidence interval would be: 50000+10−(50000−10)= $20 So, the range of this confidence interval is $20

https://docs.pyth.network/price-feeds/best-practices#confidence-intervals

From pyth docs:

(Note that $1000 is an unusually large confidence interval for bitcoin; the confidence interval is typically $50 dollars).

This would be 0.1% of bitcoin price which is unlikely to happen. In case of ETH, we can consider the max value of range of confidence to be in $10.

That attack shown as practically impossible to occur. Even if we consider this attack happening in real time, with price difference $41 for WETH and alEth than impact wouldn't be severe. Likelihood of this happening is negligible and impact is low. This at most suffice low severity.

Also I agree with @gstoyanovbg that CoinMarketCap is not an oracle and is not created to be 100% accurate. This issue can be invalidated.

Let me know your thoughts @WangSecurity

WangSecurity commented 4 months ago

Firstly, no one says that CoinMarketCap is an oracle and we can stop talking about it. Secondly, as I've said above, I understand the report talks mainly about WETH, but I believe it still identifies the core issue: using ETH price when exchange to ETH-based assets instead of ETH itself (be that WETH or alETH). And the impact is correct: bypassing slippage protection. I see that the "attack" path is not correct, but it still identifies the core issue and has a valid impact, hence, my decision remains the same: accept the escalation and duplicate with #5

gstoyanovbg commented 4 months ago

I see that the "attack" path is not correct, but it still identifies the core issue and has a valid impact, hence, my decision remains the same

@WangSecurity Doesn't this contradicts the rules ? According the rules all root cause, impact and attack path must be correct in order to have a duplicate :

Scenario A: There is a root cause/error/vulnerability A in the code. This vulnerability A -> leads to two attack paths: B -> high severity path C -> medium severity attack path/just identifying the vulnerability. Both B & C would not have been possible if error A did not exist in the first place. In this case, both B & C should be put together as duplicates. In addition to this, there is a submission D which identifies the core issue but does not clearly describe the impact or an attack path. Then D is considered low.

I am a bit confused because in the previous contest one of my reports was invalidated because of wrong attack path while root cause and impact were correct.

P.s I want to note that many people will read this report after the contest if it is marked as valid and it will not make any sense to them because is based on wrong statements. I don't think that it will put Sherlock as platform in a good light.

blckhv commented 4 months ago

I believe that historical decisions are not valid as a source of truth, and neither is adding heightened opinions about the platform's reputation. Also, escalation comments should be expressed as factual opinions, anything beyond that makes the job of the judges and everyone involved worse, so be respectful to them.

gstoyanovbg commented 4 months ago

@AydoanB I don't think I've done anything disrespectful to the judges or anyone else. I provided clear arguments based on the validity criteria rules. In the end, I also expressed my personal opinion. Historical decisions are not source of truth but there should be consistency between all similar scenarios in order to have fair contests. Of course, I don't expect it to be a reason for one judgment or another. But in a discussion, everyone is entitled to their own opinion.

WangSecurity commented 4 months ago

@gstoyanovbg as you see there's submission C in scenario A which is a valid duplicate when just identifying the vulnerability. I agree there is a thin botder between C/D in that case and it is very dependant on each case, on the issue's complexity and severity. I remember your case, but I believe in that situation the report was rather more complex and the severity was Medium. Again, I understand that it's tough to decide whether the case is C or D, since C is also just identifying the vulnerability, but the decision for this report is to accept the escalation and leave the issue as it is, since it identifies the vulnerability and explains the impact.

gstoyanovbg commented 4 months ago

@WangSecurity For me the border between C/D is pretty clear - C presents valid attack but D doesn't. Without report #5 this report doesn't make any sense, based on fabricated evidence.

I have nothing more to add and this is my last comment here. You can feel free to resolve the escalation.

WangSecurity commented 4 months ago

@gstoyanovbg for C it's not mandatory to have an attack path: "medium severity attack path/just identifying the vulnerability" which basically means "ETHEIR the medium severity attack path OR just identifying the vulnerability". I agree that without #5 it would be Low, but that's the reason report #5 is chosen as best, whereas this one is duplicate. As I've said, it depends on each case and on complexity of each case. Still as expressed here, the report identifies the vulnerability and explains valid impact, hence, the report is sufficient to being a duplicate.

Planning to accept the escalation and duplicate with #5

WangSecurity commented 4 months ago

After additional discussions, the report identifies the difference between ETH and WETH price, but in reality they're exchanged 1:1 cause WETH is a wrapper contract. Based on Pyth price feed the difference between ETH and WETH price is only $1-3, which will not cause any slippage problem. Moreover, the report mentions alETH to explain the issue in ETH <> WETH, but it doesn't indicate this problem exists with alETH. Hence, the decision is the report's not sufficient to be a duplicate of #5.

Planning to reject the escalation and leave the issue as it is.

Evert0x commented 4 months ago

Result: Invalid Unique

sherlock-admin4 commented 4 months ago

Escalations have been resolved successfully!

Escalation status:

AtanasDimulski: rejected

sherlock-audit / 2024-04-alchemix-judging

SBSecurity - `OptimismRewardCollector` uses `ETH` oracle but swap to `WETH` assuming the price is 1:1 #69

`OptimismRewardCollector` uses `ETH` oracle but swap to `WETH` assuming the price is 1:1

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

sherlock-audit / 2024-04-alchemix-judging

SBSecurity - `OptimismRewardCollector` uses `ETH` oracle but swap to `WETH` assuming the price is 1:1 #69

OptimismRewardCollector uses ETH oracle but swap to WETH assuming the price is 1:1

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

`OptimismRewardCollector` uses `ETH` oracle but swap to `WETH` assuming the price is 1:1