toshii - Malicious users can steal excess ETH in the UniswapExtension and TxBuilderExtension contracts

[sherlock-admin]

toshii

medium

Malicious users can steal excess ETH in the UniswapExtension and TxBuilderExtension contracts

Summary

A malicious user can steal any excess ETH which is stored in the UniswapExtension and TxBuilderExtension contracts due to msg.value not being cached.

Vulnerability Detail

Due to incorrect user behavior, there can be excess ETH contained within the UniswapExtension and TxBuilderExtension contracts. For example, a user might accidentally send ETH directly to the contract, which will not revert given the receive function. There is a seizeNative function which is intended to be called to rescue user funds when this happens. The issue is that a malicious user is able to steal any excess ETH in the contract prior to the admin being able to rescue these funds.

This is because (using the TxBuilderExtension as the example) the execute, and subsequently the executeInternal function accepts an array of actions to take. One potential action that can be taken is ACTION_SUPPLY_NATIVE_TOKEN, which will trigger a call to supplyNativeToken. In the supplyNativeToken function, they supply the user an amount equal to msg.value:

ironBank.supply(address(this), user, weth, msg.value);

This is an issue because a malicious user can take the following step to steal excess ETH in the contract (assume there is 1 ETH in the contract):

1. call execute with an array of actions which will trigger ACTION_SUPPLY_NATIVE_TOKEN twice with 1 ETH as the value

In the first call, it will correctly use the ETH provided by the malicious user. However, on the second call it will use the 1 ETH which is trapped in the contract. Thus, the malicious user is able to steal the ETH.

Impact

A malicious user is able to steal any excess ETH from the UniswapExtension and TxBuilderExtension contracts prior to any admin being able to rescue the funds

Code Snippet

Referenced lines of code:
https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/extensions/TxBuilderExtension.sol#L252-L256
https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/extensions/UniswapExtension.sol#L460-L464

Tool used

Manual Review

Recommendation

The msg.value value should be cached at the start of the executeInternal function call, and should be passed to all relevant functions, rather than those functions using msg.value directly.

[0xffff11]

Sponsor confirmed. Valid medium. Will be adding more duplicates previously in excluded

[0xffff11]

I correct my decision. As spoken with sponsor, the contract does not handle eth itself, therefore the only way this would make sense is if a user sense eth to the contract by error, which sherlock does not consider a medium. Low

[0xToshii]

Escalate for 10 USD
Thank you for your review. I believe that your assertion that this contract doesn't handle ETH is wrong, as many of the actions such as ACTION_SUPPLY_NATIVE_TOKEN explicitly handle ETH. Additionally, your assertion that the only way this might happen is if a user sends ETH to the contract by error is also wrong. Issue #361, which is an accepted High, shows one of many different paths that ETH will end up stuck in this contract due to normal user behavior. However, although #361 mentions that the ETH would be recoverable, in actuality a malicious user performing the attack I have laid out in this issue will be able to steal those funds.

Therefore, this is a valid Medium as per the definition: "causes a loss of funds but requires certain external conditions or specific states". Firstly, it's clear that this is a valid attack path which will result in stolen funds. Secondly, there are numerous paths in which users taking normal actions as defined by the contract will result in ETH being stuck in this contract.

[sherlock-admin]

Escalate for 10 USD
Thank you for your review. I believe that your assertion that this contract doesn't handle ETH is wrong, as many of the actions such as ACTION_SUPPLY_NATIVE_TOKEN explicitly handle ETH. Additionally, your assertion that the only way this might happen is if a user sends ETH to the contract by error is also wrong. Issue #361, which is an accepted High, shows one of many different paths that ETH will end up stuck in this contract due to normal user behavior. However, although #361 mentions that the ETH would be recoverable, in actuality a malicious user performing the attack I have laid out in this issue will be able to steal those funds.

Therefore, this is a valid Medium as per the definition: "causes a loss of funds but requires certain external conditions or specific states". Firstly, it's clear that this is a valid attack path which will result in stolen funds. Secondly, there are numerous paths in which users taking normal actions as defined by the contract will result in ETH being stuck in this contract.

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.

[stalinMacias]

Agree with the comment made by 0xToshii, adding the escalation I opened on issue #145 to complement what 0xToshii said.

[0xffff11]

Agree with escalation. As stated issue #361 is a way for eth to get stuck in the contract, which makes this attack path valid. This issue should be a medium

[ibsunhub]

fix: ibdotxyz/ib-v2#53 and ibdotxyz/ib-v2#63

[hrishibhat]

Result:
Low
Has duplicates
The contract is not supposed to hold funds.
#361 is a valid issue because it describes how eth can remain in the contract when its not supposed to.
However, this issue and its duplicates show that the funds can be stolen in a contract that's not designed to hold eth in the first place.

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• 0xtoshii: rejected