Merge pull request #91 from pessimistic-io/detector_balancer

Detector balancer

README.md

@@ -1,4 +1,4 @@
-#  Slitherin by Pessimistic.io
+# Slitherin by Pessimistic.io
 
 [![Blog](https://img.shields.io/badge/Blog-Link-blue?style=flat-square&logo=appveyor?logo=data:https://pessimistic.io/favicon.ico)](https://blog.pessimistic.io/)
 [![Our Website](https://img.shields.io/badge/By-pessimistic.io-green?style=flat-square&logo=appveyor?logo=data:https://pessimistic.io/favicon.ico)](https://pessimistic.io/)
@@ -99,6 +99,7 @@ Slitherin detectors are included into original Slither after the installation. Y
 | [Arbitrary Call](https://github.com/pessimistic-io/slitherin/blob/master/slitherin/detectors/arbitrary_call/arbitrary_call.py) | [Explore](https://github.com/pessimistic-io/slitherin/blob/master/docs/arbitrary_call.md) | [Test](https://github.com/pessimistic-io/slitherin/blob/master/tests/arbitrary_call_test.sol) | 0 |
 | [Elliptic Curve Recover](https://github.com/pessimistic-io/slitherin/blob/master/slitherin/detectors/ecrecover.py) | [Explore](https://github.com/pessimistic-io/slitherin/blob/master/docs/ecrecover.md) | [Test](https://github.com/pessimistic-io/slitherin/blob/master/tests/ecrecover.sol) | 0 |
 | [Public vs External](https://github.com/pessimistic-io/slitherin/blob/master/slitherin/detectors/public_vs_external.py) | [Explore](https://github.com/pessimistic-io/slitherin/blob/master/docs/public_vs_external.md) | [Test](https://github.com/pessimistic-io/slitherin/blob/master/tests/public_vs_external_test.sol) | 0 |
+| [Balancer Read-only Reentrancy](https://github.com/pessimistic-io/slitherin/blob/master/slitherin/detectors/balancer/balancer_readonly_reentrancy.py) | [Explore](https://github.com/pessimistic-io/slitherin/blob/master/docs/balancer/readonly_reentrancy.md) | [Test](https://github.com/pessimistic-io/slitherin/blob/master/tests/balancer/readonly_reentrancy_test.sol) | 0 |
 
 **Please note:**
 

docs/balancer/readonly_reentrancy.md

@@ -0,0 +1,24 @@
+# Balancer Readonly Reentrancy
+
+## Configuration
+
+- Check: `pess-balancer-readonly-reentrancy`
+- Severity: `High`
+- Confidence: `Medium`
+
+## Description
+
+Highlights the use of Balancer getter functions `getRate` and `getPoolTokens` (which are not checked for readonly reentrancy via `VaultReentrancyLib.ensureNotInVaultContext` or `IVault.manageUserBalance`), which return values that theoretically could be manipulated during the execution.
+
+## Vulnerable Scenario
+
+[test scenarios](../../tests/balancer/readonly_reentrancy_test.sol)
+
+## Related Attacks
+
+- [Sentimentxyz Exploit](https://quillaudits.medium.com/decoding-sentiment-protocols-1-million-exploit-quillaudits-f36bee77d376)
+- [Sturdy Exploit](https://blog.solidityscan.com/sturdy-finance-hack-analysis-bd8605cd2956)
+
+## Recommendation
+
+- [Official Balancer recomendation](https://docs.balancer.fi/concepts/advanced/valuing-bpt/valuing-bpt.html#on-chain-price-evaluation)

package.json

@@ -1,5 +1,8 @@
 {
  "dependencies": {
- "@openzeppelin/contracts": "^4.9.3"
+ "@openzeppelin/contracts": "^4.9.3",
+ "@balancer-labs/v2-interfaces": "^0.4.0",
+ "@balancer-labs/v2-pool-utils": "^4.0.0"
+
  }
 }

slitherin/__init__.py

@@ -32,6 +32,7 @@
 from slitherin.detectors.arbitrum.arbitrum_chainlink_price_feed import ArbitrumChainlinkPriceFeed
 from slitherin.detectors.potential_arith_overflow import PotentialArithmOverflow
 from slitherin.detectors.curve.curve_readonly_reentrancy import CurveReadonlyReentrancy
+from slitherin.detectors.balancer.balancer_readonly_reentrancy import BalancerReadonlyReentrancy
 from slitherin.detectors.vyper.reentrancy_curve_vyper_version import CurveVyperReentrancy
 from slitherin.detectors.price_manipulation import PriceManipulationDetector
 
@@ -67,6 +68,7 @@
  AAVEFlashloanCallbackDetector,
  PotentialArithmOverflow,
  CurveReadonlyReentrancy,
+ BalancerReadonlyReentrancy,
  CurveVyperReentrancy,
  PriceManipulationDetector
 ]

slitherin/detectors/balancer/balancer_readonly_reentrancy.py

@@ -0,0 +1,114 @@
+from typing import List
+from slither.utils.output import Output
+from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
+from slither.core.declarations import Function, Contract
+from slither.core.cfg.node import Node
+
+
+class BalancerReadonlyReentrancy(AbstractDetector):
+ """
+ Sees if a contract has a beforeTokenTransfer function.
+ """
+
+ ARGUMENT = "pess-balancer-readonly-reentrancy" # slither will launch the detector with slither.py --detect mydetector
+ HELP = "Balancer readonly-reentrancy"
+ IMPACT = DetectorClassification.HIGH
+ CONFIDENCE = DetectorClassification.MEDIUM
+
+ WIKI = "https://github.com/pessimistic-io/slitherin/blob/master/docs/balancer/readonly_reentrancy.md"
+ WIKI_TITLE = "Balancer Readonly Reentrancy"
+ WIKI_DESCRIPTION = "Check docs"
+ WIKI_EXPLOIT_SCENARIO = "-"
+ WIKI_RECOMMENDATION = "Check docs"
+
+ VULNERABLE_FUNCTION_CALLS = ["getRate", "getPoolTokens"]
+ visited = []
+ contains_reentrancy_check = {}
+
+ def is_balancer_integration(self, c: Contract) -> bool:
+ """
+ Iterates over all external function calls, and checks the interface/contract name
+ for a specific keywords to decide if the contract integrates with balancer
+ """
+ for (
+ fcontract,
+ _,
+ ) in c.all_high_level_calls:
+ contract_name = fcontract.name.lower()
+ if any(map(lambda x: x in contract_name, ["balancer", "ivault", "pool"])):
+ return True
+
+ def _has_reentrancy_check(self, node: Node) -> bool:
+ if node in self.visited:
+ return self.contains_reentrancy_check[node]
+
+ self.visited.append(node)
+ self.contains_reentrancy_check[node] = False
+
+ for c, n in node.high_level_calls:
+ if isinstance(n, Function):
+ if not n.name:
+ continue
+ if (
+ n.name == "ensureNotInVaultContext"
+ and c.name == "VaultReentrancyLib"
+ ) or (
+ n.name == "manageUserBalance"
+ ): # TODO check if errors out
+ self.contains_reentrancy_check[node] = True
+ return True
+
+ has_check = False
+ for internal_call in node.internal_calls:
+ if isinstance(internal_call, Function):
+ has_check |= self._has_reentrancy_check(internal_call)
+ # self.contains_reentrancy_check[internal_call] |= has_check
+
+ self.contains_reentrancy_check[node] = has_check
+ return has_check
+
+ def _check_function(self, function: Function) -> list:
+ has_dangerous_call = False
+ dangerous_call = None
+ for n in function.nodes:
+ for c, fc in n.high_level_calls:
+ if isinstance(fc, Function):
+ if fc.name in self.VULNERABLE_FUNCTION_CALLS:
+ dangerous_call = n # Saving only first dangerous call
+ has_dangerous_call = True
+ break
+
+ if has_dangerous_call and not any(
+ [self._has_reentrancy_check(node) for node in function.nodes]
+ ):
+ return [dangerous_call]
+ return []
+
+ def _detect(self) -> List[Output]:
+ """Main function"""
+ result = []
+ for contract in self.compilation_unit.contracts_derived:
+ if not self.is_balancer_integration(contract):
+ continue
+ res = []
+ for f in contract.functions_and_modifiers_declared:
+ function_result = self._check_function(f)
+ if function_result:
+ res.extend(function_result)
+ if res:
+ info = [
+ "Balancer readonly-reentrancy vulnerability detected in ",
+ contract,
+ ":\n",
+ ]
+ for r in res:
+ info += [
+ "\tThe answer of ",
+ r,
+ " call could be manipulated through readonly-reentrancy\n",
+ ]
+ res = self.generate_result(info)
+ res.add(r)
+ result.append(res)
+
+ return result

test_balancer.bash

@@ -0,0 +1 @@
+slither tests/balancer/readonly_reentrancy_test.sol --detect pess-balancer-readonly-reentrancy --solc-remaps @=node_modules/@

tests/balancer/readonly_reentrancy_test.sol

@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.18;
+import {VaultReentrancyLib} from "@balancer-labs/v2-pool-utils/contracts/lib/VaultReentrancyLib.sol";
+import "@balancer-labs/v2-interfaces/contracts/vault/IVault.sol";
+
+interface IBalancerPool {
+ function getRate() external view returns (uint);
+}
+
+contract BalancerIntegration {
+ address balancerVault;
+
+ constructor() {}
+
+ function getPriceVulnerable(address vault) public returns (uint) {
+ return IBalancerPool(vault).getRate();
+ }
+
+ function getPriceVulnerable2(address vault) public {
+ bytes32 poolId = "0x123";
+ uint256[] memory balances = new uint256[](10);
+ (, balances, ) = IVault(balancerVault).getPoolTokens(poolId);
+ }
+
+ function _ensureNotReentrant() internal {
+ VaultReentrancyLib.ensureNotInVaultContext(IVault(balancerVault));
+ }
+
+ function _ensureNotReentrant2() internal {
+ IVault(balancerVault).manageUserBalance(new IVault.UserBalanceOp[](0));
+ }
+
+ function getPriceOk(address vault) public returns (uint) {
+ VaultReentrancyLib.ensureNotInVaultContext(IVault(balancerVault));
+ return IBalancerPool(vault).getRate();
+ }
+
+ function getPriceOk2(address vault) public returns (uint) {
+ _ensureNotReentrant();
+ return IBalancerPool(vault).getRate();
+ }
+
+ function getPriceOk3(address vault) public returns (uint) {
+ _ensureNotReentrant2();
+ return IBalancerPool(vault).getRate();
+ }
+
+ function getPriceOk4(address vault) public returns (uint) {
+ uint a = IBalancerPool(vault).getRate();
+ _ensureNotReentrant2();
+ return a;
+ }
+
+ function getPriceOk5(address vault) public {
+ VaultReentrancyLib.ensureNotInVaultContext(IVault(balancerVault));
+ bytes32 poolId = "0x123";
+ uint256[] memory balances = new uint256[](10);
+ (, balances, ) = IVault(balancerVault).getPoolTokens(poolId);
+ }
+}