From e8fdca5e127d51c13eb3d98bf121adea12f9270c Mon Sep 17 00:00:00 2001 From: charlesndalton Date: Tue, 25 Jul 2023 22:24:35 -0400 Subject: [PATCH 1/3] fix: get chainlink to work with mismatched decimals --- .../ChainlinkExpectedOutCalculator.sol | 20 ++- milkman_py/tests/test_milkman.py | 3 +- tests/conftest.py | 144 ++++++++++++------ tests/test_complete.py | 87 ++++++++++- tests/utils.py | 2 +- 5 files changed, 203 insertions(+), 53 deletions(-) diff --git a/contracts/pricecheckers/ChainlinkExpectedOutCalculator.sol b/contracts/pricecheckers/ChainlinkExpectedOutCalculator.sol index 959ceee..94d69b5 100644 --- a/contracts/pricecheckers/ChainlinkExpectedOutCalculator.sol +++ b/contracts/pricecheckers/ChainlinkExpectedOutCalculator.sol @@ -13,6 +13,10 @@ interface IPriceFeed { function decimals() external view returns (uint8); } +interface IERC20MetaData { + function decimals() external view returns (uint8); +} + /** * @notice Checks a swap against Chainlink-compatible price feeds. * @dev Doesn't care about how long ago the price feed answer was. Another expected out calculator can be built if this is desired. @@ -47,13 +51,15 @@ contract ChainlinkExpectedOutCalculator is IExpectedOutCalculator { (address[], bool[]) ); - return getExpectedOutFromChainlink(_priceFeeds, _reverses, _amountIn); // how much Chainlink says we'd get out of this trade + return getExpectedOutFromChainlink(_priceFeeds, _reverses, _amountIn, _fromToken, _toToken); // how much Chainlink says we'd get out of this trade } function getExpectedOutFromChainlink( address[] memory _priceFeeds, bool[] memory _reverses, - uint256 _amountIn + uint256 _amountIn, + address _fromToken, + address _toToken ) internal view returns (uint256 _expectedOutFromChainlink) { uint256 _priceFeedsLen = _priceFeeds.length; @@ -85,5 +91,15 @@ contract ChainlinkExpectedOutCalculator is IExpectedOutCalculator { _scaleAnswerBy ); } + + uint256 _fromTokenDecimals = uint256(IERC20MetaData(_fromToken).decimals()); + uint256 _toTokenDecimals = uint256(IERC20MetaData(_toToken).decimals()); + + if (_fromTokenDecimals > _toTokenDecimals) { + // if fromToken has more decimals than toToken, we need to divide + _expectedOutFromChainlink = _expectedOutFromChainlink.div(_fromTokenDecimals.sub(_toTokenDecimals) ** 10); + } else if (_fromTokenDecimals < _toTokenDecimals) { + _expectedOutFromChainlink = _expectedOutFromChainlink.mul(_toTokenDecimals.sub(_fromTokenDecimals) ** 10); + } } } diff --git a/milkman_py/tests/test_milkman.py b/milkman_py/tests/test_milkman.py index e5c93fe..f76a8d1 100644 --- a/milkman_py/tests/test_milkman.py +++ b/milkman_py/tests/test_milkman.py @@ -1,5 +1,6 @@ import milkman_py + def test_simple(): print(milkman_py.curve_expected_out_data()) - assert 1 + 2 == 4 \ No newline at end of file + assert 1 + 2 == 4 diff --git a/tests/conftest.py b/tests/conftest.py index 6f2073d..0f147bb 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,7 +1,8 @@ from lib2to3.pgen2 import token from brownie import Contract from eth_abi import encode_abi -#from milkman_py import ( + +# from milkman_py import ( # univ2_expected_out_data, # univ3_expected_out_data, # curve_expected_out_data, @@ -9,11 +10,13 @@ # meta_expected_out_data, # fixed_slippage_price_checker_data, # dynamic_slippage_price_checker_data, -#) +# ) import pytest import utils EMPTY_BYTES = encode_abi(["uint8"], [int(0)]) + + def univ2_expected_out_data(): return EMPTY_BYTES @@ -78,6 +81,8 @@ def deployer(accounts): # GUSD -> USDC, $1k, Curve price checker # AAVE -> WETH, $250k, Chainlink price checker # BAT -> ALCX, $100k, Chainlink price checker +# YFI -> USDC, $20k, Chainlink price checker +# USDT -> UNI, $2M, Chainlink price checker # WETH -> WETH/BAL, $650k SSB price checker # UNI -> USDT, $500k & Uniswap as the price checker # ALCX -> TOKE, $100k, Meta price checker with Chainlink and Sushiswap @@ -97,6 +102,8 @@ def deployer(accounts): "ALCX": "0xdBdb4d16EdA451D0503b854CF79D55697F90c8DF", "BAL": "0xba100000625a3754423978a60c9317c58a424e3D", "BAL/WETH": "0x5c6Ee304399DBdB9C8Ef030aB642B10820DB8F56", + "YFI": "0x0bc529c00C6401aEF6D220BE8C6Ea1667F6Ad93e", + "USDT": "0xdAC17F958D2ee523a2206206994597C13D831ec7", } sell_to_buy_map = { @@ -109,20 +116,24 @@ def deployer(accounts): "UNI": "USDT", "ALCX": "TOKE", "BAL": "BAL/WETH", + "YFI": "USDC", + "USDT": "UNI", } @pytest.fixture( params=[ - "TOKE", - "USDC", - "GUSD", - "AAVE", - "BAT", - "WETH", - "UNI", - "ALCX", - "BAL", + # "TOKE", + # "USDC", + # "GUSD", + # "AAVE", + # "BAT", + # "WETH", + # "UNI", + # "ALCX", + # "BAL", + "YFI", + "USDT", ], scope="session", autouse=True, @@ -146,6 +157,8 @@ def token_to_buy(token_to_sell): "UNI": 80_000, "ALCX": 4_000, "BAL": 300_000, + "YFI": 3, + "USDT": 2_000_000, } @@ -172,6 +185,8 @@ def amount(token_to_sell, user, whale): "UNI": "0x1a9C8182C09F50C8318d769245beA52c32BE35BC", "ALCX": "0x000000000000000000000000000000000000dEaD", "BAL": "0x10A19e7eE7d7F8a52822f6817de8ea18204F2e4f", + "YFI": "0xFEB4acf3df3cDEA7399794D0869ef76A6EfAff52", + "USDT": "0x40ec5B33f54e0E8A33A975908C5BA1c14e5BbbDf", } @@ -197,7 +212,7 @@ def price_checker( yield sushiswap_price_checker if symbol == "USDC" or symbol == "GUSD": yield curve_price_checker - if symbol == "AAVE" or symbol == "BAT": + if symbol == "AAVE" or symbol == "BAT" or symbol == "YFI" or symbol == "USDT": yield chainlink_price_checker if symbol == "UNI": yield univ3_price_checker @@ -224,10 +239,14 @@ def sushiswap_expected_out_calculator(UniV2ExpectedOutCalculator, deployer): UniV2ExpectedOutCalculator, "SUSHI_EXPECTED_OUT_CALCULATOR", sushi_router ) + @pytest.fixture -def ssb_bal_weth_expected_out_calculator(SingleSidedBalancerBalWethExpectedOutCalculator, deployer): +def ssb_bal_weth_expected_out_calculator( + SingleSidedBalancerBalWethExpectedOutCalculator, deployer +): yield deployer.deploy(SingleSidedBalancerBalWethExpectedOutCalculator) + @pytest.fixture def univ3_expected_out_calculator(UniV3ExpectedOutCalculator, deployer): yield deployer.deploy(UniV3ExpectedOutCalculator) @@ -291,21 +310,26 @@ def meta_price_checker(DynamicSlippageChecker, meta_expected_out_calculator, dep meta_expected_out_calculator, ) + @pytest.fixture -def ssb_bal_weth_price_checker(DynamicSlippageChecker, ssb_bal_weth_expected_out_calculator, deployer): +def ssb_bal_weth_price_checker( + DynamicSlippageChecker, ssb_bal_weth_expected_out_calculator, deployer +): yield deployer.deploy( DynamicSlippageChecker, "SSB_BAL_WETH_DYNAMIC_SLIPPAGE_PRICE_CHECKER", - ssb_bal_weth_expected_out_calculator + ssb_bal_weth_expected_out_calculator, ) + @pytest.fixture def valid_from_price_checker_decorator(ValidFromPriceCheckerDecorator, deployer): - yield deployer.deploy( - ValidFromPriceCheckerDecorator - ) + yield deployer.deploy(ValidFromPriceCheckerDecorator) -def valid_from_price_checker_decorator_data(valid_from, price_checker, price_checker_data): + +def valid_from_price_checker_decorator_data( + valid_from, price_checker, price_checker_data +): return encode_abi( ["uint256", "address", "bytes"], [ @@ -314,11 +338,29 @@ def valid_from_price_checker_decorator_data(valid_from, price_checker, price_che price_checker_data, ], ) + + # which price checker data to use for each swap price_checker_datas = { "TOKE": fixed_slippage_price_checker_data(univ2_expected_out_data()), "USDC": dynamic_slippage_price_checker_data(400, curve_expected_out_data()), "GUSD": dynamic_slippage_price_checker_data(500, curve_expected_out_data()), + "YFI": dynamic_slippage_price_checker_data( + 100, + chainlink_expected_out_data( + ["0xA027702dbb89fbd58938e4324ac03B58d812b0E1"], [False] + ), + ), + "USDT": dynamic_slippage_price_checker_data( + 500, + chainlink_expected_out_data( + [ + "0xee9f2375b4bdf6387aa8265dd4fb8f16512a1d46", + "0xd6aa3d25116d8da79ea0246c4826eb951872e02e", + ], + [False, True], + ), + ), "AAVE": dynamic_slippage_price_checker_data( 400, chainlink_expected_out_data( @@ -335,10 +377,7 @@ def valid_from_price_checker_decorator_data(valid_from, price_checker, price_che [False, True], ), ), # BAT/ETH & ALCX/ETH feeds, allow 20% slippage since these are relatively illiquid - "WETH": dynamic_slippage_price_checker_data( - 200, - utils.EMPTY_BYTES - ), + "WETH": dynamic_slippage_price_checker_data(200, utils.EMPTY_BYTES), "UNI": dynamic_slippage_price_checker_data( 600, univ3_expected_out_data( @@ -351,39 +390,47 @@ def valid_from_price_checker_decorator_data(valid_from, price_checker, price_che [int(30), int(30), int(1)], ), ), # 6% slippage - "BAL": dynamic_slippage_price_checker_data(50, utils.EMPTY_BYTES), # 0.5% slippage + "BAL": dynamic_slippage_price_checker_data(50, utils.EMPTY_BYTES), # 0.5% slippage } @pytest.fixture def price_checker_data( - chain, token_to_sell, meta_price_checker, chainlink_expected_out_calculator, sushiswap_expected_out_calculator + chain, + token_to_sell, + meta_price_checker, + chainlink_expected_out_calculator, + sushiswap_expected_out_calculator, ): if token_to_sell.symbol() == "ALCX": - yield valid_from_price_checker_decorator_data(chain.time(), meta_price_checker.address, dynamic_slippage_price_checker_data( - 1000, - meta_expected_out_data( - [ - token_address["ALCX"], - token_address["WETH"], - token_address["TOKE"], - ], - [ - chainlink_expected_out_calculator.address, - sushiswap_expected_out_calculator.address, - ], - [ - encode_abi( - ["address[]", "bool[]"], - [ - ["0x194a9aaf2e0b67c35915cd01101585a33fe25caa"], - [False], - ], # forgive me father for this much nesting - ), - utils.EMPTY_BYTES, - ], + yield valid_from_price_checker_decorator_data( + chain.time(), + meta_price_checker.address, + dynamic_slippage_price_checker_data( + 1000, + meta_expected_out_data( + [ + token_address["ALCX"], + token_address["WETH"], + token_address["TOKE"], + ], + [ + chainlink_expected_out_calculator.address, + sushiswap_expected_out_calculator.address, + ], + [ + encode_abi( + ["address[]", "bool[]"], + [ + ["0x194a9aaf2e0b67c35915cd01101585a33fe25caa"], + [False], + ], # forgive me father for this much nesting + ), + utils.EMPTY_BYTES, + ], + ), ), - )) + ) else: yield price_checker_datas[token_to_sell.symbol()] @@ -394,6 +441,7 @@ def milkman(Milkman, deployer): yield milkman + @pytest.fixture def gas_checker(GasChecker, deployer): gas_checker = deployer.deploy(GasChecker) diff --git a/tests/test_complete.py b/tests/test_complete.py index ddd048a..dc8af6a 100644 --- a/tests/test_complete.py +++ b/tests/test_complete.py @@ -102,10 +102,95 @@ def test_complete_swap( assert to_bytes(is_valid_sig) == to_bytes(utils.EIP_1271_MAGIC_VALUE) - tx = gas_checker.isValidSignatureCheck(order_contract, order_digest, signature_encoded_order) + tx = gas_checker.isValidSignatureCheck( + order_contract, order_digest, signature_encoded_order + ) assert tx.gas_used < 1_000_000 +def test_bad_price( + milkman, + user, + token_to_sell, + token_to_buy, + amount, + price_checker, + price_checker_data, + chain, + hash_helper, +): + token_to_sell.approve(milkman, amount, {"from": user}) + + tx = milkman.requestSwapExactTokensForTokens( + int(amount), + token_to_sell, + token_to_buy, + user, + price_checker, + price_checker_data, + {"from": user}, + ) + + assert tx.events.count("SwapRequested") == 1 + + order_contract = Contract.from_abi( + "Milkman", tx.events["SwapRequested"]["orderContract"], milkman.abi + ) + + utils.check_swap_requested( + order_contract, + user, + user, + token_to_sell, + token_to_buy, + amount, + price_checker, + price_checker_data, + ) + + (fee_amount, buy_amount_after_fee) = utils.get_quote( + token_to_sell, token_to_buy, amount + ) + + buy_amount_after_fee = int(buy_amount_after_fee * 0.8) + + valid_to = chain.time() + 60 * 60 * 24 + + signature_encoded_order = utils.encode_order_for_is_valid_signature( + token_to_sell, + token_to_buy, + user, + user, + amount - fee_amount, + buy_amount_after_fee, + valid_to, + fee_amount, + price_checker, + price_checker_data, + ) + + # we can't create orders via API, so we need to fake it + gpv2_order = ( + token_to_sell.address, + token_to_buy.address, + user.address, + amount - fee_amount, + buy_amount_after_fee, + valid_to, + utils.APP_DATA, + fee_amount, + utils.KIND_SELL, + False, # fill or kill + utils.ERC20_BALANCE, + utils.ERC20_BALANCE, + ) + + order_digest = hash_helper.hash( + gpv2_order, to_bytes(utils.DOMAIN_SEPARATOR, "bytes32") + ) + + with brownie.reverts("invalid_min_out"): + order_contract.isValidSignature(order_digest, signature_encoded_order) # the keeper passes in order data that doesn't match the canonical order (what was used to generate the UID) def test_mismatched_order( diff --git a/tests/utils.py b/tests/utils.py index a14cbc6..d361c4b 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -314,7 +314,7 @@ def get_quote(sell_token, buy_token, sell_amount): "sellAmountBeforeFee": str(sell_amount), "priceQuality": "fast", "signingScheme": "eip1271", - "verificationGasLimit": 30000 + "verificationGasLimit": 30000, } r = requests.post(quote_url, json=post_body) From 34933cf3c7d555488951a277d876f18f5f472e20 Mon Sep 17 00:00:00 2001 From: charlesndalton Date: Wed, 26 Jul 2023 12:07:33 -0400 Subject: [PATCH 2/3] fix: correct decimal bug in Chainlink calculator --- .../ChainlinkExpectedOutCalculator.sol | 21 +++++++++++++++---- tests/conftest.py | 18 ++++++++-------- tests/test_complete.py | 18 +++++++++++++--- 3 files changed, 41 insertions(+), 16 deletions(-) diff --git a/contracts/pricecheckers/ChainlinkExpectedOutCalculator.sol b/contracts/pricecheckers/ChainlinkExpectedOutCalculator.sol index 94d69b5..b8a4203 100644 --- a/contracts/pricecheckers/ChainlinkExpectedOutCalculator.sol +++ b/contracts/pricecheckers/ChainlinkExpectedOutCalculator.sol @@ -51,7 +51,14 @@ contract ChainlinkExpectedOutCalculator is IExpectedOutCalculator { (address[], bool[]) ); - return getExpectedOutFromChainlink(_priceFeeds, _reverses, _amountIn, _fromToken, _toToken); // how much Chainlink says we'd get out of this trade + return + getExpectedOutFromChainlink( + _priceFeeds, + _reverses, + _amountIn, + _fromToken, + _toToken + ); // how much Chainlink says we'd get out of this trade } function getExpectedOutFromChainlink( @@ -92,14 +99,20 @@ contract ChainlinkExpectedOutCalculator is IExpectedOutCalculator { ); } - uint256 _fromTokenDecimals = uint256(IERC20MetaData(_fromToken).decimals()); + uint256 _fromTokenDecimals = uint256( + IERC20MetaData(_fromToken).decimals() + ); uint256 _toTokenDecimals = uint256(IERC20MetaData(_toToken).decimals()); if (_fromTokenDecimals > _toTokenDecimals) { // if fromToken has more decimals than toToken, we need to divide - _expectedOutFromChainlink = _expectedOutFromChainlink.div(_fromTokenDecimals.sub(_toTokenDecimals) ** 10); + _expectedOutFromChainlink = _expectedOutFromChainlink.div( + 10**_fromTokenDecimals.sub(_toTokenDecimals) + ); } else if (_fromTokenDecimals < _toTokenDecimals) { - _expectedOutFromChainlink = _expectedOutFromChainlink.mul(_toTokenDecimals.sub(_fromTokenDecimals) ** 10); + _expectedOutFromChainlink = _expectedOutFromChainlink.mul( + 10**_toTokenDecimals.sub(_fromTokenDecimals) + ); } } } diff --git a/tests/conftest.py b/tests/conftest.py index 0f147bb..67783d3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -123,15 +123,15 @@ def deployer(accounts): @pytest.fixture( params=[ - # "TOKE", - # "USDC", - # "GUSD", - # "AAVE", - # "BAT", - # "WETH", - # "UNI", - # "ALCX", - # "BAL", + "TOKE", + "USDC", + "GUSD", + "AAVE", + "BAT", + "WETH", + "UNI", + "ALCX", + "BAL", "YFI", "USDT", ], diff --git a/tests/test_complete.py b/tests/test_complete.py index dc8af6a..9a1d903 100644 --- a/tests/test_complete.py +++ b/tests/test_complete.py @@ -5,6 +5,7 @@ import utils from brownie.convert import to_bytes import brownie +import milkman_py def test_complete_swap( @@ -18,6 +19,7 @@ def test_complete_swap( chain, hash_helper, gas_checker, + ChainlinkExpectedOutCalculator, ): token_to_sell.approve(milkman, amount, {"from": user}) @@ -51,6 +53,7 @@ def test_complete_swap( (fee_amount, buy_amount_after_fee) = utils.get_quote( token_to_sell, token_to_buy, amount ) + amount_to_sell = amount - fee_amount valid_to = chain.time() + 60 * 60 * 24 @@ -59,7 +62,7 @@ def test_complete_swap( token_to_buy, user, user, - amount - fee_amount, + amount_to_sell, buy_amount_after_fee, valid_to, fee_amount, @@ -72,7 +75,7 @@ def test_complete_swap( token_to_sell.address, token_to_buy.address, user.address, - amount - fee_amount, + amount_to_sell, buy_amount_after_fee, valid_to, utils.APP_DATA, @@ -87,10 +90,20 @@ def test_complete_swap( gpv2_order, to_bytes(utils.DOMAIN_SEPARATOR, "bytes32") ) + # expected_out_calculator = Contract.from_abi("", price_checker.EXPECTED_OUT_CALCULATOR(), ChainlinkExpectedOutCalculator.abi) + + # expected_out_data = milkman_py.chainlink_expected_out_data( + # ["0xA027702dbb89fbd58938e4324ac03B58d812b0E1"], [False] + # ) + + # response = expected_out_calculator.getExpectedOut(amount_to_sell, token_to_sell, token_to_buy, expected_out_data) + is_valid_sig = order_contract.isValidSignature( order_digest, signature_encoded_order ) + assert to_bytes(is_valid_sig) == to_bytes(utils.EIP_1271_MAGIC_VALUE) + assert price_checker.checkPrice( amount - fee_amount, token_to_sell, @@ -100,7 +113,6 @@ def test_complete_swap( price_checker_data, ) - assert to_bytes(is_valid_sig) == to_bytes(utils.EIP_1271_MAGIC_VALUE) tx = gas_checker.isValidSignatureCheck( order_contract, order_digest, signature_encoded_order From 91ea6b00a0691c2e3c2daa1ae0174457f2f4aec0 Mon Sep 17 00:00:00 2001 From: charlesndalton Date: Wed, 26 Jul 2023 12:08:19 -0400 Subject: [PATCH 3/3] chore: lint --- contracts/periphery/GasChecker.sol | 13 ++++++++----- ...gleSidedBalancerBalWethExpectedOutCalculator.sol | 2 +- tests/test_complete.py | 3 ++- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/contracts/periphery/GasChecker.sol b/contracts/periphery/GasChecker.sol index 93224c9..0b7ae9d 100644 --- a/contracts/periphery/GasChecker.sol +++ b/contracts/periphery/GasChecker.sol @@ -3,15 +3,18 @@ pragma solidity ^0.7.6; pragma abicoder v2; interface EIP1271 { - function isValidSignature(bytes32, bytes calldata) external returns (bytes4); + function isValidSignature(bytes32, bytes calldata) + external + returns (bytes4); } /// Check that `isValidSignature` doesn't take up too much gas contract GasChecker { - function isValidSignatureCheck(address milkman, bytes32 orderDigest, bytes calldata encodedOrder) - external - returns (bytes4) - { + function isValidSignatureCheck( + address milkman, + bytes32 orderDigest, + bytes calldata encodedOrder + ) external returns (bytes4) { return EIP1271(milkman).isValidSignature(orderDigest, encodedOrder); } } diff --git a/contracts/pricecheckers/SingleSidedBalancerBalWethExpectedOutCalculator.sol b/contracts/pricecheckers/SingleSidedBalancerBalWethExpectedOutCalculator.sol index b5023e9..ae4cf51 100644 --- a/contracts/pricecheckers/SingleSidedBalancerBalWethExpectedOutCalculator.sol +++ b/contracts/pricecheckers/SingleSidedBalancerBalWethExpectedOutCalculator.sol @@ -49,7 +49,7 @@ library VaultReentrancyLib { // read-only re-entrancy protection - this call is always unsuccessful but we need to make sure // it didn't fail due to a re-entrancy attack - (, bytes memory revertData) = address(vault).staticcall{ gas: 10_000 }( + (, bytes memory revertData) = address(vault).staticcall{gas: 10_000}( abi.encodeWithSelector( vault.manageUserBalance.selector, new address[](0) diff --git a/tests/test_complete.py b/tests/test_complete.py index 9a1d903..62e0b8d 100644 --- a/tests/test_complete.py +++ b/tests/test_complete.py @@ -113,13 +113,13 @@ def test_complete_swap( price_checker_data, ) - tx = gas_checker.isValidSignatureCheck( order_contract, order_digest, signature_encoded_order ) assert tx.gas_used < 1_000_000 + def test_bad_price( milkman, user, @@ -204,6 +204,7 @@ def test_bad_price( with brownie.reverts("invalid_min_out"): order_contract.isValidSignature(order_digest, signature_encoded_order) + # the keeper passes in order data that doesn't match the canonical order (what was used to generate the UID) def test_mismatched_order( milkman,