Platform: Code4rena
Start Date: 30/10/2021
Pot Size: $35,000 ETH
Total HM: 2
Participants: 16
Period: 3 days
Judge: alcueca
Total Solo HM: 1
Id: 48
League: ETH
Rank: 2/16
Findings: 3
Award: $6,681.46
π Selected for report: 5
π Solo Findings: 0
π Selected for report: WatchPug
Also found by: daejunpark, gpersoon, hickuphh3, kenzo, pmerkleplant
hickuphh3
The executioner contract only supports ERC20<>ERC20 token trades. Native token swaps are supported by either wrapping / unwrapping the ERC20 wrapped native token before / after the trades respectively.
When exchanging from the native token, the wrapping occurs in the slingshot contract before it is transferred to the executioner contract. When exchanging to the native token, the unwrapping occurs in the executioner contract.
Examine how the initial and final balances are fetched in executeTrades()
.
uint256 initialBalance = _getTokenBalance(toToken);
if (toToken == nativeToken) { finalBalance = _getTokenBalance(address(wrappedNativeToken)); } else { finalBalance = _getTokenBalance(toToken); }
_getTokenBalance()
function _getTokenBalance(address token) internal view returns (uint256) { if (token == nativeToken) { return address(executioner).balance; } else { return IERC20(token).balanceOf(address(executioner)); } }
Notice the discrepancy between the initial and final balance fetched if toToken
is the native token. The balance before is the actual native token balance in the executioner contract, but the balance after is the wrapped token balance.
This leads to potentially wrong comparisons.
finalAmountMin
of a user.ETH_ADDRESS
(nativeToken
) as the toToken
. Alice's stuck WETH in the executioner contract are included and sent to Mallory's trade.Based on the design, the balances fetched should be from the wrapped token if toToken
is specified as the native token.
function _getTokenBalance(address token) internal view returns (uint256) { if (token == nativeToken) { return IERC20(address(wrappedNativeToken)).balanceOf(address(executioner)); } else { return IERC20(token).balanceOf(address(executioner)); } }
Then both initial and final balances can be fetched by simply calling _getTokenBalance()
uint256 initialBalance = _getTokenBalance(toToken); uint256 finalBalance = _getTokenBalance(toToken);
#0 - tommyz7
2021-11-03T15:01:55Z
Duplicate of #18
π Selected for report: hickuphh3
1994.4252 USDC - $1,994.43
hickuphh3
The executioner is designed to handle only ERC20-ERC20 token trades by modules. The balancer V2 vault is able to automatically unwrap the wrapped native token. Hence, it is recommended to ensure that the tokenOut
parameter passed into the swap()
function is not the sentinel value.
The sentinel value used is the null address.
Consider adding the following check in the function.
require(tokenOut != address(0), 'native token swap not supported');
π Selected for report: hickuphh3
1994.4252 USDC - $1,994.43
hickuphh3
Native fund transfers into the executioner contract are only expected from the wrapped token contract. Hence, it would be good to restrict incoming fund transfers to prevent accidental native fund transfers from other sources.
Modify the receive()
function to only accept transfers from the wrapped token contract.
receive() external payable { require(msg.sender == address(wrappedNativeToken), 'only wrapped native token'); }
#0 - tommyz7
2021-11-04T17:38:29Z
Duplicate of #94
π Selected for report: hickuphh3
1994.4252 USDC - $1,994.43
hickuphh3
There doesn't seem to be a use case for the existence of the receive()
function. In fact, I will recommend removing it as it will prevent accidental native token transfers to the contract.
Remove the receive()
function.
#0 - tommyz7
2021-11-04T17:37:25Z
I'm not sure in which contract, there are 2 contracts with receive()
function. One doesn't need it, the other does need it. Either way, I don't see a risk for the user of any kind. I think it's non-critical.
#1 - alcueca
2021-11-06T06:03:06Z
The added capability of the contract to receive Ether, without any purpose, is incorrect state handling. A severity of 1 is warranted.
π Selected for report: hickuphh3
Also found by: pmerkleplant
33.9293 USDC - $33.93
hickuphh3
jToken
is unused within the function, and therefore can be removed. While it can used to check against ICurvePool(curvePool).coins(j)
to ensure that the user is swapping for the right token, it isn't necessary because the final token balance is checked after all trades are executed.
Remove the jToken
input parameter.
π Selected for report: hickuphh3
75.3985 USDC - $75.40
hickuphh3
Since the modulesIndex
mapping is already declared as a public variable, the getter method isModule()
is unnecessary as it provides no extra functionality.
Rename modulesIndex
to isModule
, then remove the isModule()
function.