qbs - User can steal ETH that is stuck in contract which should be retrieved by the owner

[sherlock-admin]

qbs

medium

User can steal ETH that is stuck in contract which should be retrieved by the owner

Summary

The TxBuilderExtension contract allows an attacker to abuse the execute function to steal the Ether that is stuck within the contract.

Vulnerability Detail

The execute function in the TxBuilderExtension contract is a publicly accessible function that allows external callers to execute a series of actions. It takes an array of Action as input and invokes the internal executeInternal function.
The executeInternal function handles the execution of individual actions within the execute function. It iterates over the array of Action structures and performs different actions based on the name field of each Action.

However, let's assume that there is 1 ETH in the TxBuilderExtension contract.

1. Attacker calls TxBuilderExtension.execute with two calls to supplyNativeToken and send along 1 ETH.
2. Since there is 1 additional ETH in the contract, both calls succeed, leaving 0 ETH in the contract. Thereby the attacker has stolen the 1 ETH which only the owner should be able to rescue.

Stucked ETH can also be used for repaying position, using repayNativeToken.

Note: The same issue applies to similar functionality in the UniswapExtension contract.

Impact

Attacker can steal the ETH from the TxBuilderExtension contract that should only be accessible for the owner of the contract.

Code Snippet

https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/extensions/TxBuilderExtension.sol#L100-L102
https://github.com/sherlock-audit/2023-05-ironbank/blob/main/ib-v2/src/extensions/TxBuilderExtension.sol#L141-L197
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/TxBuilderExtension.sol#L290-L306

Tool used

Manual Review

Recommendation

Introduce additional accounting mechanisms to ensure that only the appropriate msg.value is spent.

Duplicate of #198