yAxis Metavault Review
Below is my code review of the yAxis Metavault V1 and associated contracts. This is not an audit nor an endorsement of the yAxis project. Overall I'm extremely impressed with the quality of the code.
Reviews like this can easily tilt towards a negative tone. So to start, I'd like to point out the positive aspects of the project.
- Significant portions of the code is borrowed from yearn.finance, including the concept of controllers and strategies. This is good because these patterns and code already have millions of value locked in them.
- Code is easy to read and follow, which makes it easy to review.
- Tests exist for major interactions and mocks are used for external project interactions.
- No commented out code (this is a huge pet-peeve of mine).
- All state variables have visibility modifiers (another pet-peeve).
- External contracts are not allowed to deposit to the vault. This prevents a number of economic abuse scenarios that could utilize flash loans to create unexpected behavior.
The following contracts and their associated interfaces were reviewed from the yaxis-project/metavault Github repo at commit 06929a2a1adf0be953beb69035cec7596d3a0646. Excluded from review are imported contracts from external sources (namely @openzepplin/contracts), but these are already widely used and do not need a distinct review of their usage here.
Additionally, the contracts were observed with the following addresses deployed on Ethereum mainnet.
- yAxisMetaVault (3CRV): 0xBFbEC72F2450eF9Ab742e4A27441Fa06Ca79eA6a
- yAxisMetaVaultManager: 0x9cD645330E64b07810Dde54dEe1240060071f6aa
- StrategyControllerV1: 0x2ebE1461D2Fc6dabF079882CFc51e5013BbA49B6
- StrategyPickle3Crv: 0x22F72d1D79259cE8489E912f4BF613d192000B3E
- StableSwap3PoolConverter: 0xA5c16eb6eBD72BC72c70Fca3e4faCf389AD4aBE7
Styling & Best Practices
The project makes use of common styling and best practices. The following are more nit-pick items than necessities.
- No Natspec documentation available in the contract source code.
- Mixed use of uint and uint256, should conform to uint256.
- Tests rely on previous tests to run in order to be successful. This is because the state of the chain doesn't reset after each test runs (before vs beforeEach or using a snapshot of the chain for each test).
Governance at the time of this writing is set to the following address for MetaVault(3CRV), StrategyControllerV1, and StableSwap3PoolConverter: 0xC1d40e197563dF727a4d3134E8BD1DeF4B498C6f
Governance for StrategyPickle3Crv and yAxisMetaVaultManager is set to the yAxis Deployer account: 0x5661bF295f48F499A70857E8A6450066a8D16400
- Code duplication betwen deposit and depositAll functions (lines 255-266 & 293-304) could be shared with an internal function to shave some bytecode.
- Would really like to see some tests around getMultiplier.
- The following lines in the
unstakefunction are redundant. SafeMath will catch the require line. Instead, the second line could make use of SafeMath's overloaded
subfunction accepting the
string memory errorMessageas the third param:
require(user.amount >= _amount, "stakedBal < _amount"); user.amount = user.amount.sub(_amount);
- IMetaVault has imports that need to be removed. These imports are already declared in yAxisMetaVault and do not need to be included in the interface file.
- Does not implement IVaultManager. This doesn't seem to be a problem because I hand-checked each function is there.
- No tests exist specifically for this contract.
- No tests exist specifically for this contract, though functions of this contract are used in other tests. I would still like to see tests for the setters at a minimum.
- getExpectedReturn (from OneSplitAudit) doesn't seem to be used anywhere in the project. This is brought over from yearn vaults and can possibly be removed.
- This contract uses an in-line interface for Converter to implement one
convert(address)function. This could easily be missed and should probably be moved into its own file.
- Question: is allowing the strategist the ability to set the vault address for a token too much permission for that role? Advice: only governance should be able to call this function.
- No tests for this specific contract.
- It would be nice to see
type(uint256).maxused in this contract (like in the constructor for StrategyPickle3Crv) instead of
- Addresses for want, p3crv, pickle, weth, t3crv, dai, usdc, and usdt appear to be hardcoded, but are also supplied as parameters to the constructor. This makes it possible for mistakes to be made when deploying where what readers of the contract code will expect one address, but the deployer sets a different address. I went through and made sure that the StrategyPickle3Crv contract deployed at address 0x22F72d1D79259cE8489E912f4BF613d192000B3E was deployed with the correct addreses, but in the future it would be better to simply use the
immutablekeyword and only rely on supplying the addresses on deployment. This gives another benefit of not using storage, since immutable variables are constants declared in the constructor.
- No tests exist for the setter functions. While it's obvious in the Solidity code that only governance can update state variables, it's better in the long run to prevent unexpected changes in the future.
- Does not implement IStrategy.