Due to multiple issues the reinvestor may be able to steal funds from the vault.
Summary
Due to combining multiple issues the reinvestor may be able to steal funds from the vault.
Vulnerability Detail
Issue 1 - reinvest reward possible with rewardToken==ETH:
ConvexStakingMixin._isInvalidRewardToken() is only checking against Deployments.ALT_ETH_ADDRESS but is missing a check against Deployments.ETH_ADDRESS (address(0)).
This may result in the reinvestor to use SingleSidedLPVaultBase.reinvestReward() to reinvest ETH (address(0)).
Issue 2:
TradingUtils._approve() approves WETH if trade.sellToken == address(0). Note: It can only approve WETH up to the balance of ETH the vault is holding. Note: reinvesting is possible with rewardToken==ETH (address(0)) due to Issue 1.
Issue 3:
TokenUtils.checkRevoke() not revoking WETH when sellToken is ETH. This leads to a lingering WETH approval to the DEX.
Issue 4:
ZeroExAdapter.getExecutionData() not validating trade.exchangeData. The reinvestor can specify any exchangeData which may open up malicious tx.
Potential issue with malicious reinvestor when executing reinvestReward():
Let's assume we have this constellation:
TOKEN_1 = WETH
TOKEN_2 != ETH
rewardToken = ETH
trade.sellToken = Deployments.ETH_ADDRESS
DEX = ZERO_EX
reinvestor tx1 (reinvestReward()):
specify ZERO_EX
sellToken ETH
buyToken TOKEN_1 or TOKEN_2
with exchangeData:
but sell nothing
slippage 0
This will approve WETH to ZERO_EX
revoke will not happen.
reinvestor tx2:
specify ZERO_EX again
trade.sellToken = a token which is not TOKEN_1 or TOKEN_2 and which has a balance in the vault or which i send to the vault
trade.amount = just set it to a small amount so it doesn't revert
exchangeData: this is the real data that the exchange is using
sell WETH as much as is approved from the vault
since approval already happened in tx1
receiver = reinvestor
ALSO NOTE: no slippage protection if WETH goes back to vault.
Note: Sorry I am writing this last minute, so I didn't have the time for more formatting.
Impact
The reinvestor steals WETH by selling WETH from the vault in the 2nd tx and receives other assets from ZERO_EX as part of the trade.
Due to multiple issues there is an attack vector for a reinvestor to steal funds from the protocol by using ZERO_EX.
Currently this attack only works if the vault is holding ETH, because when WETH is being approved for Zero_EX, the protocol only approves up to the balance of ETH in the vault.
Due to multiple issues that were listed above there may be an attack vector when the vault is holding ETH which can be abused by the reinvestor to steal all WETH from the vault by trading it via multiple reinvestReward() tx to Zero_EX, specifying the reinvestor address as receiver.
Code Snippet
Tool used
Manual Review
Recommendation
Consider fixing the issues listed above with approval of WETH and lingering approval of WETH which is not revoked under the circumstances described. Also consider sanitizing exchangeData when trading with Zero_Ex.
lemonmon
high
Due to multiple issues the reinvestor may be able to steal funds from the vault.
Summary
Due to combining multiple issues the reinvestor may be able to steal funds from the vault.
Vulnerability Detail
Issue 1 - reinvest reward possible with rewardToken==ETH:
Deployments.ALT_ETH_ADDRESSbut is missing a check againstDeployments.ETH_ADDRESS(address(0)).This may result in the reinvestor to use
SingleSidedLPVaultBase.reinvestReward()to reinvest ETH (address(0)).Issue 2:
TradingUtils._approve()approvesWETHiftrade.sellToken == address(0). Note: It can only approve WETH up to the balance of ETH the vault is holding. Note: reinvesting is possible with rewardToken==ETH (address(0)) due to Issue 1.Issue 3:
TokenUtils.checkRevoke()not revoking WETH when sellToken is ETH. This leads to a lingering WETH approval to the DEX.Issue 4:
Potential issue with malicious reinvestor when executing
reinvestReward():Let's assume we have this constellation:
TOKEN_1 = WETH
TOKEN_2 != ETH
rewardToken = ETH
trade.sellToken = Deployments.ETH_ADDRESS
DEX = ZERO_EX
reinvestor tx1 (
reinvestReward()):specify ZERO_EX
sellToken ETH
buyToken TOKEN_1 or TOKEN_2
with exchangeData:
but sell nothing
slippage 0
This will approve WETH to ZERO_EX
revoke will not happen.
reinvestor tx2:
specify ZERO_EX again
trade.sellToken = a token which is not TOKEN_1 or TOKEN_2 and which has
a balance in the vault or which i send to the vault
trade.amount = just set it to a small amount so it doesn't revert
exchangeData: this is the real data that the exchange is using
sell WETH as much as is approved from the vault
since approval already happened in tx1
receiver = reinvestor
ALSO NOTE: no slippage protection if WETH goes back to vault.
Note: Sorry I am writing this last minute, so I didn't have the time for more formatting.
Impact
The reinvestor steals WETH by selling WETH from the vault in the 2nd tx and receives other assets from ZERO_EX as part of the trade.
Due to multiple issues there is an attack vector for a reinvestor to steal funds from the protocol by using ZERO_EX.
Currently this attack only works if the vault is holding ETH, because when WETH is being approved for Zero_EX, the protocol only approves up to the balance of ETH in the vault.
Due to multiple issues that were listed above there may be an attack vector when the vault is holding ETH which can be abused by the reinvestor to steal all WETH from the vault by trading it via multiple reinvestReward() tx to Zero_EX, specifying the reinvestor address as receiver.
Code Snippet
Tool used
Manual Review
Recommendation
Consider fixing the issues listed above with approval of WETH and lingering approval of WETH which is not revoked under the circumstances described. Also consider sanitizing exchangeData when trading with Zero_Ex.
Duplicate of #74