Sommelier Finance Audit Notes

# Sommelier Finance Audit Notes **Auditor:** Jake Bunce **Client:** Sommelier Finance https://sommelier.finance/ https://github.com/VolumeFi/cellars/tree/b887510055d96f2dc32a56d0143fa8846d3a07c8/contracts **Commit:** `b887510055d96f2dc32a56d0143fa8846d3a07c8` ## Whitepaper & specification about the project Whitepaper: https://docs.zora.co/zoraos/dev/smart-contracts/whitepaper Project is a trading platform: Sommelier is a bet that Ethereum will be a dominant player in the global economy. Sommelier consists of the Cosmos Stargate SDK, its Tendermint-based consensus layer and a decentralized, bi-directional Ethereum bridge, managed by a global network of validators. Liquidity Providers (LPs) will be able to use the Sommelier to author and execute complex, and automated financial transactions, such as portfolio rebalancing, limit orders, batched orders, as well as a host of other features that traders have come to expect from CeFi, but that are not currently available in DeFi. ## Review of the protocol/implementation **[1] Unlocked Pragma** **Files Affected:** `contracts/CellarPoolShare.sol`, `contracts/CellarPoolShareLimitETHUSDT.sol`, `contracts/CellarPoolShareLimitUSDCETH.sol`, `contracts/interfaces.sol` **Severity: Low** Default AL text. **Recommendations:** Default AL text. **[2] Own implementation of reentrancy guard** **Files Affected:** `contracts/CellarPoolShare.sol` **Severity: Low** Reentrancy protection has been defined via a custom implementation. **Recommendations:** Consider using a library that is well audited and enjoys wide deployment instead of rolling your own. Further reading: https://docs.openzeppelin.com/contracts/4.x/api/security#ReentrancyGuard **[3] Greedy Contract** **Files Affected:** `contracts/CellarPoolShare.sol` **Severity: Medium** The contract will accept Ether from users but there is no way for them to retrieve this from the contract. **Recommendations:** Prevent Ether from being deposited into the contract by adding a `revert()` into the fallback function. **[4] Management Function Argument Validation** **Files Afected:** `contracts/CellarPoolShare.sol`, `contracts/CellarPoolShareLimitETHUSDT.sol`, **Severity: Medium** [`setValidator()`](https://github.com/VolumeFi/cellars/blob/b887510055d96f2dc32a56d0143fa8846d3a07c8/contracts/CellarPoolShare.sol#L577), [`transferOwnership()`](https://github.com/VolumeFi/cellars/blob/b887510055d96f2dc32a56d0143fa8846d3a07c8/contracts/CellarPoolShare.sol#L582), [`setManagementFee()`](https://github.com/VolumeFi/cellars/blob/b887510055d96f2dc32a56d0143fa8846d3a07c8/contracts/CellarPoolShare.sol#L587), [`setPerformanceFee()`](https://github.com/VolumeFi/cellars/blob/b887510055d96f2dc32a56d0143fa8846d3a07c8/contracts/CellarPoolShare.sol#L592), do not check for the validity of the address argument is sound. **Recommendations:** Verify that the `newOwner` argument is not the zero address. **[5] Inconsistent implementations for liquidity addition** **Files Affected:** `contracts/CellarPoolShare.sol`, `contracts/CellarPoolShareLimitETHUSDT.sol`, `contracts/CellarPoolShareLimitUSDCETH.sol` **Severity: High** `addLiquidityForUniV3()` is inconsistency between `CellarPoolShare.sol` and `CellarPoolShareLimitETHUSDT.sol` & `CellarPoolShareLimitUSDCETH` with the total supply and pricing. **Recommendations:** Move to a single function to remove code duplication and clarify why there are limits for some assets but not others. **[6] Balances can be over/underflowed** **Files Affected:** `contracts/CellarPoolShare.sol`, `contracts/CellarPoolShareLimitETHUSDT.sol`, `contracts/CellarPoolShareLimitUSDCETH.sol`. **Severity: High** Integer values can be over/underflowed. This means values could be manipulated to incorrect values by an attacker. **Recommendations:** Use the OpenZeppelin library [`SafeMath`](https://docs.openzeppelin.com/contracts/2.x/api/math) with < `0.8.0` of Solidity, or move to >= `0.8.0` where such protection is built into the compiler. **[7] Incomplete Function** **Files Affected:** `contracts/CellarPoolShare.sol` **Severity: Medium** [`_beforeTokenTransfer()`](https://github.com/VolumeFi/cellars/blob/b887510055d96f2dc32a56d0143fa8846d3a07c8/contracts/CellarPoolShare.sol#L300) appears to be an incomplete function. **Recommendations:** Implement the function or clarify the intention behind this function **[8] Unclear Operation/Lack of Documentation** **Files Affected:** `contracts/CellarPoolShare.sol`, `contracts/CellarPoolShareLimitETHUSDT.sol`, `contracts/CellarPoolShareLimitUSDCETH.sol`. **Severity: Undefined** [`invest()`](https://github.com/VolumeFi/cellars/blob/b887510055d96f2dc32a56d0143fa8846d3a07c8/contracts/CellarPoolShare.sol#L279) and weight operations [`_getWeightInfo()`](https://github.com/VolumeFi/cellars/blob/b887510055d96f2dc32a56d0143fa8846d3a07c8/contracts/CellarPoolShare.sol#L708) & [`_modifyWeightInfo()`](https://github.com/VolumeFi/cellars/blob/b887510055d96f2dc32a56d0143fa8846d3a07c8/contracts/CellarPoolShare.sol#L840) are complicated and there's no supporting documentation. Please provide documentation to describe the intended outcome of these functions. Additionally there is duplication of these functions across the Solidity files and there should only be one. **[9] Clarify Flash loan Protection** **Files Affected:** `contracts/interfaces.sol` **Severity: Undefined** Contract [`BlockLock`](https://github.com/VolumeFi/cellars/blob/b887510055d96f2dc32a56d0143fa8846d3a07c8/contracts/interfaces.sol#L1184) is used for protection against flash loans. Please provide documentation on the intended behaviour of this protection. **[10] Multiple instances of reentrancy** **Files Affected:** `contracts/CellarPoolShare.sol`, `contracts/CellarPoolShareLimitETHUSDT.sol`, `contracts/CellarPoolShareLimitUSDCETH.sol` **Severity: High** [`invest()`](https://github.com/VolumeFi/cellars/blob/b887510055d96f2dc32a56d0143fa8846d3a07c8/contracts/CellarPoolShare.sol#L279), [`invest()`](https://github.com/VolumeFi/cellars/blob/b887510055d96f2dc32a56d0143fa8846d3a07c8/contracts/CellarPoolShareLimitETHUSDT.sol#L277), [`invest()`](https://github.com/VolumeFi/cellars/blob/b887510055d96f2dc32a56d0143fa8846d3a07c8/contracts/CellarPoolShareLimitUSDCETH.sol#L277) are all susceptible to reentrancy. **Recommendations:** Implement reentrancy protection for this function. Suggested to use an open source library that is well audited and enjoys a deployment from a number of projects, such as https://docs.openzeppelin.com/contracts/4.x/api/security#ReentrancyGuard. ## Tests ``` ================================= test session starts ================================== platform linux -- Python 3.6.9, pytest-6.2.5, py-1.10.0, pluggy-1.0.0 rootdir: /app plugins: eth-brownie-1.16.4, web3-5.23.1, xdist-1.34.0, forked-1.3.0, hypothesis-6.21.6 collected 16 items Attached to local RPC client listening at '127.0.0.1:8545'... tests/test_00_add_liquidity.py .. [ 12%] tests/test_01_transfer.py ... [ 31%] tests/test_02_remove_liquidity.py ... [ 50%] tests/test_03_reinvest.py ... [ 68%] tests/test_04_rebalance.py ... [ 87%] tests/test_05_weight.py . [ 93%] tests/test_06_add_liquidity_limit.py . [100%] =================================== warnings summary =================================== ../home/ethsec/.local/pipx/venvs/eth-brownie/lib/python3.6/site-packages/brownie/network/main.py:46 /home/ethsec/.local/pipx/venvs/eth-brownie/lib/python3.6/site-packages/brownie/network/main.py:46: BrownieEnvironmentWarning: Development network has a block height of 13416770 BrownieEnvironmentWarning, -- Docs: https://docs.pytest.org/en/stable/warnings.html ====================== 16 passed, 1 warning in 359.90s (0:05:59) ======================= ``` ## Best Practices **[1] Lint Errors** ``` CellarPoolShare.sol 19:5 warning Explicitly mark visibility of state state-visibility 19:5 warning 'NONFUNGIBLEPOSITIONMANAGER' should start with _ private-vars-leading-underscore 22:5 warning Explicitly mark visibility of state state-visibility 22:5 warning 'UNISWAPV3FACTORY' should start with _ private-vars-leading-underscore 25:5 warning Explicitly mark visibility of state state-visibility 25:5 warning 'SWAPROUTER' should start with _ private-vars-leading-underscore 28:5 warning Explicitly mark visibility of state state-visibility 28:5 warning 'WETH' should start with _ private-vars-leading-underscore 30:5 warning Explicitly mark visibility of state state-visibility 30:5 warning 'FEEDOMINATOR' should start with _ private-vars-leading-underscore 32:5 warning Explicitly mark visibility of state state-visibility 32:5 warning 'YEAR' should start with _ private-vars-leading-underscore 50:5 warning Explicitly mark visibility of state state-visibility 50:5 warning 'lastManageTimestamp' should start with _ private-vars-leading-underscore 93:5 warning Function order is incorrect, modifier definition can not go after constructor (line 60) ordering 135:5 warning Function body contains 105 lines but allowed no more than 50 lines function-max-lines 135:5 warning Function has cyclomatic complexity 14 but allowed no more than 7 code-complexity 142:9 warning Provide an error message for require reason-string 142:17 warning Avoid to make time-based decisions in your business logic not-rely-on-time 246:9 warning Provide an error message for require reason-string 246:17 warning Avoid to make time-based decisions in your business logic not-rely-on-time 279:5 warning Function body contains 110 lines but allowed no more than 50 lines function-max-lines 279:5 warning 'invest' should start with _ private-vars-leading-underscore 338:15 warning Code contains empty blocks no-empty-blocks 338:24 warning Code contains empty blocks no-empty-blocks 370:15 warning Code contains empty blocks no-empty-blocks 370:24 warning Code contains empty blocks no-empty-blocks 399:5 warning 'getManagementFee' should start with _ private-vars-leading-underscore 415:5 warning Function body contains 77 lines but allowed no more than 50 lines function-max-lines 415:5 warning Function has cyclomatic complexity 8 but allowed no more than 7 code-complexity 422:28 warning Avoid to make time-based decisions in your business logic not-rely-on-time 474:31 warning Avoid to make time-based decisions in your business logic not-rely-on-time 495:5 warning Function body contains 79 lines but allowed no more than 50 lines function-max-lines 495:5 warning Function has cyclomatic complexity 10 but allowed no more than 7 code-complexity 515:10 warning Variable "outAmount0" is unused no-unused-vars 515:30 warning Variable "outAmount1" is unused no-unused-vars 515:50 warning Variable "liquiditySum" is unused no-unused-vars 517:31 warning Avoid to make time-based decisions in your business logic not-rely-on-time 708:5 warning Function body contains 114 lines but allowed no more than 50 lines function-max-lines 840:5 warning Function body contains 66 lines but allowed no more than 50 lines function-max-lines 917:5 warning Function body contains 133 lines but allowed no more than 50 lines function-max-lines 1029:29 warning Code contains empty blocks no-empty-blocks 1042:29 warning Code contains empty blocks no-empty-blocks 1063:5 warning Function body contains 71 lines but allowed no more than 50 lines function-max-lines 1073:28 warning Avoid to make time-based decisions in your business logic not-rely-on-time 1141:24 warning Code contains empty blocks no-empty-blocks 1143:5 warning All public or external methods in a contract must override a definition from an interface comprehensive-interface 1143:32 warning Code contains empty blocks no-empty-blocks CellarPoolShareLimitETHUSDT.sol 12:5 warning Explicitly mark visibility of state state-visibility 12:5 warning 'NONFUNGIBLEPOSITIONMANAGER' should start with _ private-vars-leading-underscore 15:5 warning Explicitly mark visibility of state state-visibility 15:5 warning 'UNISWAPV3FACTORY' should start with _ private-vars-leading-underscore 18:5 warning Explicitly mark visibility of state state-visibility 18:5 warning 'SWAPROUTER' should start with _ private-vars-leading-underscore 21:5 warning Explicitly mark visibility of state state-visibility 21:5 warning 'WETH' should start with _ private-vars-leading-underscore 23:5 warning Explicitly mark visibility of state state-visibility 23:5 warning 'FEEDOMINATOR' should start with _ private-vars-leading-underscore 25:5 warning Explicitly mark visibility of state state-visibility 25:5 warning 'YEAR' should start with _ private-vars-leading-underscore 42:5 warning Explicitly mark visibility of state state-visibility 42:5 warning 'lastManageTimestamp' should start with _ private-vars-leading-underscore 45:5 warning Constant name must be in capitalized SNAKE_CASE const-name-snakecase 80:5 warning Function order is incorrect, modifier definition can not go after constructor (line 47) ordering 122:5 warning 'totalPrice' should start with _ private-vars-leading-underscore 128:5 warning Function body contains 116 lines but allowed no more than 50 lines function-max-lines 128:5 warning Function has cyclomatic complexity 15 but allowed no more than 7 code-complexity 135:9 warning Provide an error message for require reason-string 135:17 warning Avoid to make time-based decisions in your business logic not-rely-on-time 250:9 warning Provide an error message for require reason-string 250:17 warning Avoid to make time-based decisions in your business logic not-rely-on-time 277:5 warning Function body contains 102 lines but allowed no more than 50 lines function-max-lines 277:5 warning 'invest' should start with _ private-vars-leading-underscore 331:15 warning Code contains empty blocks no-empty-blocks 331:24 warning Code contains empty blocks no-empty-blocks 360:15 warning Code contains empty blocks no-empty-blocks 360:24 warning Code contains empty blocks no-empty-blocks 382:5 warning 'getManagementFee' should start with _ private-vars-leading-underscore 398:5 warning Function body contains 77 lines but allowed no more than 50 lines function-max-lines 398:5 warning Function has cyclomatic complexity 8 but allowed no more than 7 code-complexity 405:28 warning Avoid to make time-based decisions in your business logic not-rely-on-time 457:31 warning Avoid to make time-based decisions in your business logic not-rely-on-time 478:5 warning Function body contains 79 lines but allowed no more than 50 lines function-max-lines 478:5 warning Function has cyclomatic complexity 10 but allowed no more than 7 code-complexity 498:10 warning Variable "outAmount0" is unused no-unused-vars 498:30 warning Variable "outAmount1" is unused no-unused-vars 498:50 warning Variable "liquiditySum" is unused no-unused-vars 500:31 warning Avoid to make time-based decisions in your business logic not-rely-on-time 680:5 warning Function body contains 107 lines but allowed no more than 50 lines function-max-lines 790:5 warning Function body contains 66 lines but allowed no more than 50 lines function-max-lines 859:5 warning Function body contains 133 lines but allowed no more than 50 lines function-max-lines 971:29 warning Code contains empty blocks no-empty-blocks 984:29 warning Code contains empty blocks no-empty-blocks 995:5 warning Function body contains 71 lines but allowed no more than 50 lines function-max-lines 1005:28 warning Avoid to make time-based decisions in your business logic not-rely-on-time 1073:24 warning Code contains empty blocks no-empty-blocks 1075:5 warning All public or external methods in a contract must override a definition from an interface comprehensive-interface 1075:32 warning Code contains empty blocks no-empty-blocks CellarPoolShareLimitUSDCETH.sol 12:5 warning Explicitly mark visibility of state state-visibility 12:5 warning 'NONFUNGIBLEPOSITIONMANAGER' should start with _ private-vars-leading-underscore 15:5 warning Explicitly mark visibility of state state-visibility 15:5 warning 'UNISWAPV3FACTORY' should start with _ private-vars-leading-underscore 18:5 warning Explicitly mark visibility of state state-visibility 18:5 warning 'SWAPROUTER' should start with _ private-vars-leading-underscore 21:5 warning Explicitly mark visibility of state state-visibility 21:5 warning 'WETH' should start with _ private-vars-leading-underscore 23:5 warning Explicitly mark visibility of state state-visibility 23:5 warning 'FEEDOMINATOR' should start with _ private-vars-leading-underscore 25:5 warning Explicitly mark visibility of state state-visibility 25:5 warning 'YEAR' should start with _ private-vars-leading-underscore 42:5 warning Explicitly mark visibility of state state-visibility 42:5 warning 'lastManageTimestamp' should start with _ private-vars-leading-underscore 45:5 warning Constant name must be in capitalized SNAKE_CASE const-name-snakecase 80:5 warning Function order is incorrect, modifier definition can not go after constructor (line 47) ordering 122:5 warning 'totalPrice' should start with _ private-vars-leading-underscore 128:5 warning Function body contains 116 lines but allowed no more than 50 lines function-max-lines 128:5 warning Function has cyclomatic complexity 15 but allowed no more than 7 code-complexity 135:9 warning Provide an error message for require reason-string 135:17 warning Avoid to make time-based decisions in your business logic not-rely-on-time 250:9 warning Provide an error message for require reason-string 250:17 warning Avoid to make time-based decisions in your business logic not-rely-on-time 277:5 warning Function body contains 102 lines but allowed no more than 50 lines function-max-lines 277:5 warning 'invest' should start with _ private-vars-leading-underscore 331:15 warning Code contains empty blocks no-empty-blocks 331:24 warning Code contains empty blocks no-empty-blocks 360:15 warning Code contains empty blocks no-empty-blocks 360:24 warning Code contains empty blocks no-empty-blocks 382:5 warning 'getManagementFee' should start with _ private-vars-leading-underscore 398:5 warning Function body contains 77 lines but allowed no more than 50 lines function-max-lines 398:5 warning Function has cyclomatic complexity 8 but allowed no more than 7 code-complexity 405:28 warning Avoid to make time-based decisions in your business logic not-rely-on-time 457:31 warning Avoid to make time-based decisions in your business logic not-rely-on-time 478:5 warning Function body contains 79 lines but allowed no more than 50 lines function-max-lines 478:5 warning Function has cyclomatic complexity 10 but allowed no more than 7 code-complexity 498:10 warning Variable "outAmount0" is unused no-unused-vars 498:30 warning Variable "outAmount1" is unused no-unused-vars 498:50 warning Variable "liquiditySum" is unused no-unused-vars 500:31 warning Avoid to make time-based decisions in your business logic not-rely-on-time 680:5 warning Function body contains 107 lines but allowed no more than 50 lines function-max-lines 790:5 warning Function body contains 66 lines but allowed no more than 50 lines function-max-lines 859:5 warning Function body contains 133 lines but allowed no more than 50 lines function-max-lines 971:29 warning Code contains empty blocks no-empty-blocks 984:29 warning Code contains empty blocks no-empty-blocks 995:5 warning Function body contains 71 lines but allowed no more than 50 lines function-max-lines 1005:28 warning Avoid to make time-based decisions in your business logic not-rely-on-time 1073:24 warning Code contains empty blocks no-empty-blocks 1075:5 warning All public or external methods in a contract must override a definition from an interface comprehensive-interface 1075:32 warning Code contains empty blocks no-empty-blocks interfaces.sol 568:5 warning 'RESOLUTION' should start with _ private-vars-leading-underscore 569:5 warning 'Q96' should start with _ private-vars-leading-underscore 575:2 error Line length must be no more than 120 but current length is 129 max-line-length 576:2 error Line length must be no more than 120 but current length is 123 max-line-length 578:2 error Line length must be no more than 120 but current length is 127 max-line-length 584:5 warning Function body contains 94 lines but allowed no more than 50 lines function-max-lines 604:13 warning Provide an error message for require reason-string 613:9 warning Provide an error message for require reason-string 689:5 warning 'MIN_TICK' should start with _ private-vars-leading-underscore 692:5 warning 'MAX_TICK' should start with _ private-vars-leading-underscore 697:2 error Line length must be no more than 120 but current length is 125 max-line-length 699:5 warning Function body contains 59 lines but allowed no more than 50 lines function-max-lines 699:5 warning Function has cyclomatic complexity 21 but allowed no more than 7 code-complexity 770:5 warning 'toUint128' should start with _ private-vars-leading-underscore 771:9 warning Provide an error message for require reason-string 780:5 warning Function order is incorrect, internal function can not go after private function (line 770) func-order 780:5 warning Function order is incorrect, internal pure function can not go after private pure function (line 770) ordering 926:1 warning Function order is incorrect, interface can not go after library definition (line 766) ordering 1186:5 warning 'BLOCK_LOCK_COUNT' should start with _ private-vars-leading-underscore ✖ 167 problems (4 errors, 163 warnings) ``` **[2] Filename capitalisation** `interfaces.sol` -> `Interfaces.sol` for consistency **[3] Lack of docstrings** Functions should be well documented with docstrings defining intended operation. **[4] Unclear Error Codes** Many functions have `require()` clauses to evaluate conditions prior to execution. The error returned to the user is in the format `RX`, where X is an integer. It would be better for users to have a meaningful error message returned instead of a number.