diff --git a/README.md b/README.md index 983d9fa..6bdc9ad 100644 --- a/README.md +++ b/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:** diff --git a/docs/balancer/readonly_reentrancy.md b/docs/balancer/readonly_reentrancy.md new file mode 100644 index 0000000..7094fb8 --- /dev/null +++ b/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) diff --git a/package-lock.json b/package-lock.json index cc36f30..8fb9792 100644 --- a/package-lock.json +++ b/package-lock.json @@ -5,9 +5,33 @@ "packages": { "": { "dependencies": { + "@balancer-labs/v2-interfaces": "^0.4.0", + "@balancer-labs/v2-pool-utils": "^4.0.0", "@openzeppelin/contracts": "^4.9.3" } }, + "node_modules/@balancer-labs/v2-interfaces": { + "version": "0.4.0", + "resolved": "https://registry.npmjs.org/@balancer-labs/v2-interfaces/-/v2-interfaces-0.4.0.tgz", + "integrity": "sha512-K0ij26m8/UOvdPmrAnuh/C7kT8OQupsgV8KRyIt+aTHW1KbPOi4v8zLMwW2AwSYMSRjPK2A/ttlnNizT0iA4Qg==" + }, + "node_modules/@balancer-labs/v2-pool-utils": { + "version": "4.1.1", + "resolved": "https://registry.npmjs.org/@balancer-labs/v2-pool-utils/-/v2-pool-utils-4.1.1.tgz", + "integrity": "sha512-CZRXqD71Sog7zZh8ycdAN1Q0RpYPTvH7orQcWUNbj9ooWc5M7eqmgntsK8Me1a5AKroxf6rH0xLQBYoBaapbMA==", + "dependencies": { + "@balancer-labs/v2-interfaces": "0.4.0", + "@balancer-labs/v2-solidity-utils": "4.0.0" + } + }, + "node_modules/@balancer-labs/v2-solidity-utils": { + "version": "4.0.0", + "resolved": "https://registry.npmjs.org/@balancer-labs/v2-solidity-utils/-/v2-solidity-utils-4.0.0.tgz", + "integrity": "sha512-BB9bawgywV8q8v/m3xhYVI5X0rJ3ox64t4IYiAQvgdrR7+Sgn6AAkAP++odjbDJdCcM5wQMi92zwt50rq2nn6A==", + "dependencies": { + "@balancer-labs/v2-interfaces": "0.4.0" + } + }, "node_modules/@openzeppelin/contracts": { "version": "4.9.3", "resolved": "https://registry.npmjs.org/@openzeppelin/contracts/-/contracts-4.9.3.tgz", @@ -15,6 +39,28 @@ } }, "dependencies": { + "@balancer-labs/v2-interfaces": { + "version": "0.4.0", + "resolved": "https://registry.npmjs.org/@balancer-labs/v2-interfaces/-/v2-interfaces-0.4.0.tgz", + "integrity": "sha512-K0ij26m8/UOvdPmrAnuh/C7kT8OQupsgV8KRyIt+aTHW1KbPOi4v8zLMwW2AwSYMSRjPK2A/ttlnNizT0iA4Qg==" + }, + "@balancer-labs/v2-pool-utils": { + "version": "4.1.1", + "resolved": "https://registry.npmjs.org/@balancer-labs/v2-pool-utils/-/v2-pool-utils-4.1.1.tgz", + "integrity": "sha512-CZRXqD71Sog7zZh8ycdAN1Q0RpYPTvH7orQcWUNbj9ooWc5M7eqmgntsK8Me1a5AKroxf6rH0xLQBYoBaapbMA==", + "requires": { + "@balancer-labs/v2-interfaces": "0.4.0", + "@balancer-labs/v2-solidity-utils": "4.0.0" + } + }, + "@balancer-labs/v2-solidity-utils": { + "version": "4.0.0", + "resolved": "https://registry.npmjs.org/@balancer-labs/v2-solidity-utils/-/v2-solidity-utils-4.0.0.tgz", + "integrity": "sha512-BB9bawgywV8q8v/m3xhYVI5X0rJ3ox64t4IYiAQvgdrR7+Sgn6AAkAP++odjbDJdCcM5wQMi92zwt50rq2nn6A==", + "requires": { + "@balancer-labs/v2-interfaces": "0.4.0" + } + }, "@openzeppelin/contracts": { "version": "4.9.3", "resolved": "https://registry.npmjs.org/@openzeppelin/contracts/-/contracts-4.9.3.tgz", diff --git a/package.json b/package.json index b6f9903..016662d 100644 --- a/package.json +++ b/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" + } } diff --git a/slitherin-benchmark b/slitherin-benchmark index e8a7b64..e7224b8 160000 --- a/slitherin-benchmark +++ b/slitherin-benchmark @@ -1 +1 @@ -Subproject commit e8a7b64b52af91eb257399cdaf16c68c5695d9d7 +Subproject commit e7224b87fc769bee913e59dc1976dfd7de7bd209 diff --git a/slitherin/__init__.py b/slitherin/__init__.py index eed8f62..7298975 100644 --- a/slitherin/__init__.py +++ b/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 ] diff --git a/slitherin/detectors/balancer/balancer_readonly_reentrancy.py b/slitherin/detectors/balancer/balancer_readonly_reentrancy.py new file mode 100644 index 0000000..4dba564 --- /dev/null +++ b/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 diff --git a/test_balancer.bash b/test_balancer.bash new file mode 100755 index 0000000..2a15042 --- /dev/null +++ b/test_balancer.bash @@ -0,0 +1 @@ +slither tests/balancer/readonly_reentrancy_test.sol --detect pess-balancer-readonly-reentrancy --solc-remaps @=node_modules/@ \ No newline at end of file diff --git a/tests/balancer/readonly_reentrancy_test.sol b/tests/balancer/readonly_reentrancy_test.sol new file mode 100644 index 0000000..77499fb --- /dev/null +++ b/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); + } +}