0x007 - msg.sender == tx.origin does not protect against attacks like sandwich

[sherlock-admin]

0x007

medium

msg.sender == tx.origin does not protect against attacks like sandwich

Summary

According to the codebase, some functions are restricted to EOA to stop flash loan and sandwich attacks. However, this solution is not thorough for the following reasons

• Sandwich can be done on L1 using tools like flashbots to submit bundles.
• Institutions are trying their hands on MEV, so capital might not be restriction for enticing opportunities that involve sandwich.
• msg.sender == tx.origin does not imply EOA on L2 chains when the transaction is sent from L1
• EOA might not be only address that can start transactions in the future on ethereum because of ongoing discussion and plans on account abstraction.

Vulnerability Detail

Impact

The smart contract is attacked from onlyEOA functions such as rebalance, iterateRebalance, ripcord

Code Snippet

https://github.com/sherlock-audit/2023-05-Index/blob/main/index-coop-smart-contracts/contracts/lib/BaseExtension.sol#L56-L62 https://github.com/sherlock-audit/2023-05-Index/blob/main/index-coop-smart-contracts/contracts/adapters/AaveLeverageStrategyExtension.sol#L304 https://github.com/sherlock-audit/2023-05-Index/blob/main/index-coop-smart-contracts/contracts/adapters/AaveLeverageStrategyExtension.sol#L338 https://github.com/sherlock-audit/2023-05-Index/blob/main/index-coop-smart-contracts/contracts/adapters/AaveLeverageStrategyExtension.sol#L376

Tool used

Manual Review

Recommendation

Make sure the smart contract is resilient no matter the msg.sender or tx.origin.

[sherlock-admin]

Escalate for 10 USDC

I believe this is valid cause my argument is that every transaction worth sandwiching would be sandwiched and thereby extracting value from depositors.

The 3 functions that use onlyEOA--rebalance, iterateRebalance, ripcord--have 2 things in common which are public access and swapping tokens. And from the protocol design, it seemed to trust caller and onlyEOA protection to some extent.

• it always use slippageTolerance to calculate min output token
• caller specify exchange
• all exchanges uses the same slippageTolerance even though their conditions like liquidity, swap formula might be different
• rebalance and iterateRebalance might not use allowlist, in which case the attacker or MEV searcher could also be the caller.
• ripcord is the best endpoint for attack because
  • it doesn't use allowlist and absolutely anyone could call it
  • it uses incentivizedSlippageTolerance which would likely be higher than slippageTolerance
  • the caller also provides the exchange to use
• It permits stale price from latestAnswer

A safe assumption is that the slippageTolerance would always be reached during swaps and setting the perfect value would be hard cause on-chain conditions such as liquidity around price does change frequently and those functions might be called multiple times a day.

The attacks or conditions I'm most worried about are the ones I don't even know of. An example is 0x52 issue about chainlink slippage tolerance. Other concerns are non-swap MEV.

Some other issues also mention onlyEOA and points listed here like L2 chains and eip-3074, but this might be unique cause

• the real impact or threat is not mentioned
• Attack cause of L2 chains and eip-3074 are very less likely than sandwich and institutional MEV
• Their recommendation of using isContract makes the attack more easy with flashloan. Because isContract would be false if the code is executed from constructor.

There's no simple recommendation and the decision would depend on Index's risk tolerance and code complexity tolerance

• Enforcing allowlist for rebalance and iterateRebalance and accepting a minimum output from the parameter would still leave ripcord vulnerable
• Using different slippageTolerance for different exchanges might help. But setting the right slippageTolerance would be difficult cause if it is too small, it would be hard to rebalance cause they would revert often. A value that's decently ok or large takes value from depositors to MEV searchers.
• Off-chain techniques like monitoring on-chain events and lots of due diligence on enabled exchanges might also help.

You've deleted an escalation for this issue.

[bizzyvinci]

Escalate for 10 USDC

I believe this is valid cause my argument is that every transaction worth sandwiching would be sandwiched and thereby extracting value from depositors.

The 3 functions that use onlyEOA--rebalance, iterateRebalance, ripcord--have 2 things in common which are public access and swapping tokens. And from the protocol design, it seemed to trust caller and onlyEOA protection to some extent.

• it always use slippageTolerance to calculate min output token
• caller specify exchange
• all exchanges uses the same slippageTolerance even though their conditions like liquidity, swap formula might be different
• rebalance and iterateRebalance might not use allowlist, in which case the attacker or MEV searcher could also be the caller.
• ripcord is the best endpoint for attack because
  • it doesn't use allowlist and absolutely anyone could call it
  • it uses incentivizedSlippageTolerance which would likely be higher than slippageTolerance
  • the caller also provides the exchange to use
• It permits stale price from latestAnswer

A safe assumption is that the slippageTolerance would always be reached during swaps and setting the perfect value would be hard cause on-chain conditions such as liquidity around price does change frequently and those functions might be called multiple times a day.

The attacks or conditions I'm most worried about are the ones I don't even know of. An example is 0x52 issue about chainlink slippage tolerance. Other concerns are non-swap MEV.

Some other issues also mention onlyEOA and points listed here like L2 chains and eip-3074, but this might be unique cause

• the real impact or threat is not mentioned
• Attack cause of L2 chains and eip-3074 are very less likely than sandwich and institutional MEV
• Their recommendation of using isContract makes the attack more easy with flashloan. Because isContract would be false if the code is executed from constructor.

There's no simple recommendation and the decision would depend on Index's risk tolerance and code complexity tolerance

• Enforcing allowlist for rebalance and iterateRebalance and accepting a minimum output from the parameter would still leave ripcord vulnerable
• Using different slippageTolerance for different exchanges might help. But setting the right slippageTolerance would be difficult cause if it is too small, it would be hard to rebalance cause they would revert often. A value that's decently ok or large takes value from depositors to MEV searchers.
• Off-chain techniques like monitoring on-chain events and lots of due diligence on enabled exchanges might also help.

[sherlock-admin]

Escalate for 10 USDC

I believe this is valid cause my argument is that every transaction worth sandwiching would be sandwiched and thereby extracting value from depositors.

The 3 functions that use onlyEOA--rebalance, iterateRebalance, ripcord--have 2 things in common which are public access and swapping tokens. And from the protocol design, it seemed to trust caller and onlyEOA protection to some extent.

• it always use slippageTolerance to calculate min output token
• caller specify exchange
• all exchanges uses the same slippageTolerance even though their conditions like liquidity, swap formula might be different
• rebalance and iterateRebalance might not use allowlist, in which case the attacker or MEV searcher could also be the caller.
• ripcord is the best endpoint for attack because
  • it doesn't use allowlist and absolutely anyone could call it
  • it uses incentivizedSlippageTolerance which would likely be higher than slippageTolerance
  • the caller also provides the exchange to use
• It permits stale price from latestAnswer

A safe assumption is that the slippageTolerance would always be reached during swaps and setting the perfect value would be hard cause on-chain conditions such as liquidity around price does change frequently and those functions might be called multiple times a day.

The attacks or conditions I'm most worried about are the ones I don't even know of. An example is 0x52 issue about chainlink slippage tolerance. Other concerns are non-swap MEV.

Some other issues also mention onlyEOA and points listed here like L2 chains and eip-3074, but this might be unique cause

• the real impact or threat is not mentioned
• Attack cause of L2 chains and eip-3074 are very less likely than sandwich and institutional MEV
• Their recommendation of using isContract makes the attack more easy with flashloan. Because isContract would be false if the code is executed from constructor.

There's no simple recommendation and the decision would depend on Index's risk tolerance and code complexity tolerance

• Enforcing allowlist for rebalance and iterateRebalance and accepting a minimum output from the parameter would still leave ripcord vulnerable
• Using different slippageTolerance for different exchanges might help. But setting the right slippageTolerance would be difficult cause if it is too small, it would be hard to rebalance cause they would revert often. A value that's decently ok or large takes value from depositors to MEV searchers.
• Off-chain techniques like monitoring on-chain events and lots of due diligence on enabled exchanges might also help.

You've created a valid escalation for 10 USDC!

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.

[Evert0x]

Although the Watson demonstrates how to game the tx origin check it fails to show a clear example on how this will impact the protocol.

@0xffff11 what do you think?

[bizzyvinci]

A safe assumption is that the slippageTolerance would always be reached during swaps and setting the perfect value would be hard cause on-chain conditions such as liquidity around price does change frequently and those functions might be called multiple times a day.

Lets assume the protocol configures a max slippage tolerance of 2% for a non stable pair like USDT/LINK--which seems reasonable.

My argument is that this is a gold mine for MEV Searchers and this is what they could do.

• Make sure they monitor the status of the extension so that they would be the one to always call rebalance.
• Sandwich the rebalance trade but make sure it doesn't deviate from the 2% tolerance.
• Rinse and repeat.

This process could happen multiple times a day and thereby gradually extracting value from the SetToken.

https://github.com/sherlock-audit/2023-05-Index/blob/main/index-coop-smart-contracts/contracts/lib/BaseExtension.sol#L56-L62

/**
     * Throws if caller is a contract, can be used to stop flash loan and sandwich attacks
     */
    modifier onlyEOA() {
        require(msg.sender == tx.origin, "Caller must be EOA Address");
        _;
    }

https://github.com/sherlock-audit/2023-05-Index/blob/main/index-coop-smart-contracts/contracts/adapters/AaveLeverageStrategyExtension.sol#L304

function rebalance(string memory _exchangeName) external onlyEOA onlyAllowedCaller(msg.sender) {

https://github.com/sherlock-audit/2023-05-Index/blob/main/index-coop-smart-contracts/contracts/adapters/AaveLeverageStrategyExtension.sol#L338

function iterateRebalance(string memory _exchangeName) external onlyEOA onlyAllowedCaller(msg.sender) {

https://github.com/sherlock-audit/2023-05-Index/blob/main/index-coop-smart-contracts/contracts/adapters/AaveLeverageStrategyExtension.sol#L376

function ripcord(string memory _exchangeName) external onlyEOA {

[bizzyvinci]

On a second thought, it might not be as lucrative as I'd imagined cause operator has a lot of flexibility with exchange configuration

https://github.com/sherlock-audit/2023-05-Index/blob/main/index-coop-smart-contracts/contracts/adapters/AaveLeverageStrategyExtension.sol#L112

struct ExchangeSettings {
      uint256 twapMaxTradeSize;                        // Max trade size in collateral base units
      uint256 exchangeLastTradeTimestamp;              // Timestamp of last trade made with this exchange
      uint256 incentivizedTwapMaxTradeSize;            // Max trade size for incentivized rebalances in collateral base units
      bytes leverExchangeData;                         // Arbitrary exchange data passed into rebalance function for levering up
      bytes deleverExchangeData;                       // Arbitrary exchange data passed into rebalance function for delevering
  }

[hrishibhat]

@bizzyvinci it is unclear how this would impact the protocol in addition to what you mention here. Seems like an informational issue.

[bizzyvinci]

Yes, I agree it be invalidated

[hrishibhat]

Result: Low Unique Considering this a non issue based on above comments

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• bizzyvinci: rejected