This could lead to different problems:
1) If someone (accidentally) sends ETH to the executioner then:
1a) "finalBalance - initialBalance" could be negative, resulting in a revert
1b) finalBalance - initialBalance could be smaller than finalAmountMin, resulting in a revert (even if the expected amount of wrappedNativeToken has been received)
So you could create a grieving attack by sending ETH to the executioner
Then any future trades with toToken == nativeToken will most likely fail (until the nativeToken is removed with rescueTokens)
2) You can sweep any available wrappedNativeToken in the executioner because executioner.balance and thus initialBalance will usually be 0,
so finalBalance is the entire amount of wrappedNativeToken in the executioner.
Possible solutions:
1) Calculate initialBalance in the same way as finalBalance:
if (toToken == nativeToken) {
initialBalance = _getTokenBalance(address(wrappedNativeToken));
} else {
initialBalance = _getTokenBalance(toToken);
}
2) Sum the amount of nativeToken and wrappedNativeToken:
function _getTokenBalance(address token) internal view returns (uint256 balance) {
if (token == nativeToken) {
balance = address(executioner).balance;
token = address(wrappedNativeToken);
}
balance += IERC20(token).balanceOf(address(executioner));
}
3) Ignore the initialBalance because the executioner isn't supposed to have any tokens anyway
Handle
gpersoon
Vulnerability details
Impact
The function executeTrades() of Slingshot.sol calulates the initialBalance and finalBalance in a different way. If toToken == nativeToken then:
This could lead to different problems: 1) If someone (accidentally) sends ETH to the executioner then: 1a) "finalBalance - initialBalance" could be negative, resulting in a revert 1b) finalBalance - initialBalance could be smaller than finalAmountMin, resulting in a revert (even if the expected amount of wrappedNativeToken has been received) So you could create a grieving attack by sending ETH to the executioner Then any future trades with toToken == nativeToken will most likely fail (until the nativeToken is removed with rescueTokens) 2) You can sweep any available wrappedNativeToken in the executioner because executioner.balance and thus initialBalance will usually be 0, so finalBalance is the entire amount of wrappedNativeToken in the executioner.
Proof of Concept
https://github.com/code-423n4/2021-10-slingshot/blob/f6e7a0a39e3267bbe3c7fe60d6074cbf54f5750f/contracts/Slingshot.sol#L81-L91
Tools Used
Recommended Mitigation Steps
Possible solutions: 1) Calculate initialBalance in the same way as finalBalance: if (toToken == nativeToken) { initialBalance = _getTokenBalance(address(wrappedNativeToken)); } else { initialBalance = _getTokenBalance(toToken); }
2) Sum the amount of nativeToken and wrappedNativeToken: function _getTokenBalance(address token) internal view returns (uint256 balance) { if (token == nativeToken) { balance = address(executioner).balance;
token = address(wrappedNativeToken); } balance += IERC20(token).balanceOf(address(executioner)); }
3) Ignore the initialBalance because the executioner isn't supposed to have any tokens anyway