Slingshot Finance contest - hickuphh3's results

Web3 trading platform.

General Information

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

Slingshot Finance

Findings Distribution

Researcher Performance

Rank: 2/16

Findings: 3

Award: $6,681.46

🌟 Selected for report: 5

πŸš€ Solo Findings: 0

Findings Information

🌟 Selected for report: WatchPug

Also found by: daejunpark, gpersoon, hickuphh3, kenzo, pmerkleplant

Labels

bug
duplicate
2 (Med Risk)

Awards

588.8441 USDC - $588.84

External Links

Handle

hickuphh3

Vulnerability details

Impact

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().

Initial Balance

uint256 initialBalance = _getTokenBalance(toToken);

Final Balance

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.

Effects

This leads to potentially wrong comparisons.

  1. A malicious user can cause griefing by sending native tokens directly to the executioner contract (note that it has a receiver function) that exceeds the finalAmountMin of a user.
  2. Wrapped tokens that are accidentally sent to the executioner contract can be exploited as they can be considered to be part of the attacker's trade output. For instance,
    • Alice accidentally sends WETH to the executioner contract
    • Mallory specifies a minimal swap with 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

Findings Information

🌟 Selected for report: hickuphh3

Labels

bug
1 (Low Risk)
sponsor confirmed

Awards

1994.4252 USDC - $1,994.43

External Links

Handle

hickuphh3

Vulnerability details

Impact

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');

Findings Information

🌟 Selected for report: hickuphh3

Labels

bug
1 (Low Risk)
sponsor confirmed

Awards

1994.4252 USDC - $1,994.43

External Links

Handle

hickuphh3

Vulnerability details

Impact

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

Findings Information

🌟 Selected for report: hickuphh3

Labels

bug
1 (Low Risk)
disagree with severity

Awards

1994.4252 USDC - $1,994.43

External Links

Handle

hickuphh3

Vulnerability details

Impact

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.

Findings Information

🌟 Selected for report: hickuphh3

Also found by: pmerkleplant

Labels

bug
G (Gas Optimization)
sponsor acknowledged

Awards

33.9293 USDC - $33.93

External Links

Handle

hickuphh3

Vulnerability details

Impact

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.

Findings Information

🌟 Selected for report: hickuphh3

Labels

bug
G (Gas Optimization)
sponsor acknowledged

Awards

75.3985 USDC - $75.40

External Links

Handle

hickuphh3

Vulnerability details

Impact

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.

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax Β© 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter