Skip to content

Instantly share code, notes, and snippets.

@transferAndCall

transferAndCall/yaxis.md

Last active November 10, 2020 17:26
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save transferAndCall/41e1b46241eb854025a440d6485815d7 to your computer and use it in GitHub Desktop.
Save transferAndCall/41e1b46241eb854025a440d6485815d7 to your computer and use it in GitHub Desktop.
Download ZIP
Raw
yaxis.md

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.

Positives

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.

Contracts Reviewed

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.

  • yAxisMetaVault.sol
  • yAxisMetaVaultManager.sol
  • StrategyControllerV1.sol
  • StrategyPickle3Crv.sol
  • StableSwap3PoolConverter.sol

Additionally, the contracts were observed with the following addresses deployed on Ethereum mainnet.

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).

Ownership

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

Contracts

yAxisMetaVault

  • 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 unstake function are redundant. SafeMath will catch the require line. Instead, the second line could make use of SafeMath's overloaded sub function accepting the string memory errorMessage as 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.

yAxisMetaVaultManager

  • 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.

StrategyControllerV1

  • 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.

StableSwap3PoolConverter

  • No tests for this specific contract.
  • It would be nice to see type(uint256).max used in this contract (like in the constructor for StrategyPickle3Crv) instead of uint(-1).

StrategyPickle3Crv

  • 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 immutable keyword 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment