search

sherlock-audit / 2022-09-notional-judging

4 stars 2 forks source link

lemonmon - `TradingUtils::_executeTrade` will leak ETH to WETH #98

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

lemonmon

high

TradingUtils::_executeTrade will leak ETH to WETH

Summary

If sellToken is ETH, and using Uniswap for the dex, and it is exact out trade, too much is deposited to the WETH and does not withdraw the excess amount. It will give wrong amountSold value as well as accounting error.

Vulnerability Detail

trade.sellToken is ETH and using Uniswap as dex, WETH should be used instead of ETH as Uniswap does not support ETH. There for TradingUtils wraps the ETH to WETH before trading.

If the trade would be exact out, the amount trade.limit will be deposited to WETH instead of the trade.amount. However, because it is exact out, not all ETH deposited will be traded. In the current implementation, there is no logic to recover the excess deposit.

As the TradingUtils::_executeInternal, which uses the TradingUtils::_executeTrade will calculate the amountSold based on the balance of ETH, it will return the trade.limit as the amountSold, thus resulting in accounting error.

Note: in the current implementation, the trade using Uniswap with ETH as sellToken would not even work, because the WETH is not properly approved (issue 2). This issue assumes that the issue is resolved.

Impact

amountSold will reflect not the amount really sold, rather the trade.limit. It is unclear whether the excess amount of ETH, which is deposited for WETH can be recovered.

Code Snippet

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/trading/TradingUtils.sol?plain=1#L118-L137

https://github.com/sherlock-audit/2022-09-notional/blob/main/leveraged-vaults/contracts/trading/TradingUtils.sol?plain=1#L29-L64

Tool used

Manual Review

Recommendation

In the _executeTrade, if the sellToken is ETH and it is exact out trade, recover excess deposit.

jeffywu commented 1 year ago

@Evert0x I don't think this is a duplicate of #110

weitianjie2000 commented 1 year ago

legit issue, will be fixed

rayn731 commented 1 year ago

LGTM. The fix follows the suggetion. it would unwrap excess WETH if trade.sellToken == Deployments.ETH_ADDRESS && spender != Deployments.ETH_ADDRESS && _isExactOut(trade).