lyra-finance / v2-core

Core smart contracts for the Lyra V2 Protocol

Lyra V2

The framework to trade all derivatives

Build

git submodule update --init --recursive
forge build

Tests

forge test

Go to test folder for more details on how to run different tests.

Documentation

Go to docs to understand the high-level design, transaction flow, and how different Lyra V2 components work together.

Static Analysis - Slither

Please go to the link above for detail to install. To run the analysis:

slither src

Deployment

Got to scripts to understand how to deploy Lyra v2 to different networks.

v2-core's Issues

Avoid checking "same sign" through multiplication

https://github.com/lyra-finance/v2-core/blob/4b84365837678d40a12ba24891337c4614d7774e/src/libraries/PCRMGrouping.sol#L67

My bad on POCing it this way in python code, but I guess we should stick to a proper isSameSign(x,y) function of some sort, or ((x<0) == (y<0));, because of overflow risk and probably a larger gas overhead.

name collision (duplicated function) in `ConvertDecimals` and `DecimalMath`

proposal: PR and branch guideline

@0xdomrom @antoncoding With Proto1 approaching, I'd like to propose a some general structure for how we organize our branches and PRs in V2. I expect this to evolve over time but believe it will be helpful to setup some expectations before things get hectic.

Here are some general concepts + a visual diagram reference below:

1. Three four branch types:
   (a) The main branch where tested production code lives (this is what is expected to be deployed upon launch)
   (b) 1x workstream branch per workstream (this represents the complete set of feature complete WS contracts)
   (c) 1x testing branch per workstream
   (d) 2-4x smaller feature branches per workstream (help break up PRs to not overwhelm reviewers)
2. Feature->workstream PRs only require 1x peer review while workstream->main PRs require 2x peers.
3. Workstream->main PRs must be "ready-for-review" on week 5
4. Code-freeze: no more code changes that affect other workstreams are permitted (week 6).

Smart Contractor Org Working Deck

Would love to hear your comments with the expectation that we would evolve this structure as we trial it in P1.

audit: make sure you check all assets in manager hook

Any address calling assetAdjustment with adjustment.asset == msg.sender can add an invalid asset entry and prevent subsequent calls to changeManager. Recommend investigating a global whitelist of assets, functionality to abandon assets and EIP-165 interface detection for assets.

Our manager should be able to handle weird assets by rejecting the final state, but it's not implemented yet

System setup todos

Things we have to make sure setup correctly after deployment

• Make sure ChangeManagerModule from Matching is a trusted risk assessor.
• Ensure the Matching and related contracts are never removed from the allowList, which will prevent
  forceWithdraw() from being called on accounts held in those contracts (SigP Lyra-16)
• setFeeBypassedCaller for matching

Import Ordering Styleguide

The solidity style guide doesn't have much guidance on the proper import scheme.

Here's my suggestion loosely based off of Google's C++ guide:

1. Any major system level import (e.g. forge-std/Script, hardhat/console.sol)
2. External contracts / libraries: should be already audited (e.g. open-zeppelin, chainlink, synthetix)
3. Internal libraries (e.g. DecimalMath.sol)
4. Internal contracts (e.g. Accounts.sol)

proposal: testing guideline

@0xdomrom @joshpwrk After looking at the codebase, I want to propose something that adds more clarity to testing structure (process) and hopefully help with review process here:

What I see mostly done in the test folder are some tests between unit test and integration tests:

• they are not unit tests because we actually code bunch of logic in the "mock contracts" that interact with it, instead of only "mocking the return"
• they are not integration tests because they're not real modules that we wish the test the whole flow

I'd personally call these "POC tests": where we write sloppy code to test our hypothesis about:

• if the interface make sense
• if the whole workflow make sense
• what would the actual integrating modules "look like" (for example, the OptionToken and QuoteWrapper)

These are very important tests and I think we will each write bunch of them while designing the system in a modular way.

Problem

However the problem with "only having these kind of tests" is that it is very hard to "setup" a scenario and test some basic logic and make sure we make no mistake in some trivial things. If we're aiming for full coverage, this is gonna post a lot of unnecessary work.

Take allowance tests for example: see we have such a complicated setup, even deploying Lending in deployPRMSystem while I just want to test if allowance is working correctly with positive value / negative value, and if someone can steal money without proper allowance. Also if we add more "mock contracts" in the future, we will have to update these tests as well. (and the gas benchmark is gonna be inaccurate too and at some point we will ignore all of them)

function setUp() public {
    deployPRMSystem();
    setPrices(1e18, 1500e18);

    PortfolioRiskManager.Scenario[] memory scenarios = new PortfolioRiskManager.Scenario[](1);
    scenarios[0] = PortfolioRiskManager.Scenario({spotShock: uint(85e16), ivShock: 10e18});

    setScenarios(scenarios);

    aliceAcc = createAccountAndDepositUSDC(alice, 10000000e18);
    bobAcc = createAccountAndDepositUSDC(bob, 10000000e18);
  }

Proposal

I think we should have a separate folder of unit tests which should be very very simple to tests all the if-else branch, and make sure the function work as intended.
Take allowance for example, we just need to deploy Accounts, and a super easy Asset that add money into the account system, and we check if another user can update the account balance without allowance.

Ideally the setup should be as easy as

function setUp() public {
    usdc = new TestERC20("usdc", "USDC");
    usdcAsset = new QuoteWrapper(IERC20(usdc), account);

    account = new Account("Lyra Margin Accounts", "LyraMarginNFTs");

    aliceAcc = createAccountAndDepositUSDC(alice, 10000000e18);
    bobAcc = createAccountAndDepositUSDC(bob, 10000000e18);
    
  }

Where I don't need to know about what are scenarios or every contract inside deployPRMSystem, nor do i need to test things with a "OptionAdaptor".

This will make sure we have a set of tests that only "describe the behavior" of a single contract, and doesn't have any dependencies on anything else. In the future we don't have to change every single unit tests when we have a new "POC for OptionToken".

This is also to make sure we can look at the "unit tests" and see what the function are supposed to work, without knowing how the rest of the system works. Ideally when we start writing functions, we use these some super fast unit tests that make sure all the math, storage updates and access logic are taken care of, as a way to prove of correctness to the reviewers, and we use "POC tests" to show people what the rest of the world should look like (extremely important when we're still having some moving pieces with the design).

I think by having the consensus about different kind of tests, we can dramatically reduce the burden on both the coder and the reviewer. As we start to take care of our own modules and work in pipeline, it would become more and more time consuming for us to review each others' code if we require the knowledge about all moving pieces to review 1 simple test.

Next step:

What I plan on doing is I can have a guideline setup (including naming and folder separation) so we have consensus about what kind of tests are more of a "POC tests" that we don't have to care too much about until it's mark as final, and which are unit tests that best describe whats being implemented in a certain PR. This is also gonna be helpful for auditors to come in and look at our code.

I can add the guide line and start separating some unit tests for Accounts, hopefully code will speak more straightforward

audit: changeManager permissions

It seems strange that a manager is permitted to invoke changeManager. Should changeManager not be limited to onlyOwnerOrApproved?

Orderbook Related Events

https://github.com/lyra-finance/v2-core/blob/c7e12ee712429c1908ee59bf23086109b9ec4ca0/src/assets/CashAsset.sol#L281

In the orderbook, we'd like to easily figure out when a user earned interest and how much. accrueInterest only specifies the total interest accrued, but we cannot back-out exactly how much a user paid for a premium vs accrued in interest during a tx.

audit: should add returned variable on hooks

Hooks should have a return value that is validated to ensure that the hook successfully executed. If the asset or manager has a fallback function it could seem as if the hook successfully executed when it did not (especially if it is behind an upgradeable proxy).

0x318820b2962E130725f91a42919BF6256e188350

https://github.com/lyra-finance/v2-core/blob/70e1819825367085ab6f4a9246fb60f267430e4f/src/SubAccounts.sol#L259

Griefing attack in Lyra V2

Bug Description

Where
permitAndSubmitTransfer and permitAndSubmitTransfers

Expected behavior:

• The permitAndSubmitTransfers function utilizes the lyra’s permit function so that approve and pull operations can happen in a single transaction instead of two consecutive transactions.

Attack:

• _Permit () functionality uses the nonces mapping for replay protection. Once a signature is verified and approved, the nonce increases, invalidating the same signature being replayed.
• permitAndSubmitTransfers expects the holder to sign their tokens and provide the signature to contract as part of permitData
  When a permitAndSubmitTransfers transaction is in the mempool, an attacker can take this signature, call the external permit() function on the token themselves.
• Since this is a valid signature, the token accepts it and increases the nonce.
• This makes the spender's transaction fail whenever it gets mined.

Impact

• Attacker can make sure all calls to permitAndSubmitTransfers fail for the first time.
• Approve and pull operations can not happen in a single transaction.

Risk Breakdown

• Difficulty to Exploit: Easy
• Severity: Medium

Recommendation

• In permitAndSubmitTransfers fuction, check if it has the approval it needs. If not, then only submit the permit signature.

Reference

• 1Inch:
  https://github.com/1inch/solidity-utils/blob/master/contracts/libraries/SafeERC20.sol#L299
• Gearbox:
  https://github.com/Gearbox-protocol/core-v3/blob/main/contracts/credit/CreditFacadeV3.sol#L775

chore: move synthetix folder out of `lib`

lib/synthetix stores contracts we copied and modified from synthetix, but it's not a submodules. We should move it to src/libraries/ after sprint 1

audit: check in createAccount

When creating an account, there is no validation that the manager is valid. Recommend validating the manager using EIP-165 or with a whitelist of approved managers.

Deployment script TODOs

• Deploy to multiple networks
• Configs per network
• Save deployed contract addresses and ABIs
• Be able to fetch contract objects for deployed contracts and execute functions easily

security: revisit `createAndApprove`

Potential vulnerability: opens up "phishing attack" opportunities for a malicious user to create a position for another user and add "approve himself" to update the position. If the targeted user then deposit into the account, the fund could be lost

Todo: ask some auditors and get their feedback before audit

chore: duplicated `Deposit` event in `IERC20BasedAsset` and `ICashAsset`

We should just use 1 version, defines it in IERC20BaseAsset to avoid some confusion on the client side

PCRM: Make spot shocks symmetric

    uint timeWeight = timeSlope.multiplyDecimal(timeToExpiry * 1e18 / 365 days);

    uint upShock = spotUpPercent + timeWeight;
    uint downShock = (spotDownPercent > timeWeight) ? spotDownPercent - timeWeight : 0;

to:

    uint timeWeight = DecimalMath.UNIT + timeSlope.multiplyDecimal(timeToExpiry * 1e18 / 365 days);

    uint upShock = spotUpPercent.multiplyDecimal(timeWeight);
    uint downShock = spotDownPercent.divideDecimal(timeWeight);

make all `IxxLyraFeed` inherit `IBaseLyraFeed`

Rename futureFeed to forwardFeed

Nitpick, but it is important nonetheless - we can't call the feed "futures feed" because we are not trading or offering futures. We deal with forwards / forward prices.

• Futures imply daily settlement of cashflows
• Forwards imply one settlement at expiry (what we have)

It is confusing / misleading otherwise.

add getSpot for perps

feat: include a signing scheme in `Accounts.sol`

Similar to Uniswap's permit2, should add a signing scheme into the accounts layer to remove any unnecessary approvals

chore: bump coverage for `ArrayLib.sol` and `IntLib.sol`

Let the coverage report pick up by writing explicit unit tests that follow the scheme here

make oracles updatable

No reason to make them immutable now, we want to have the flexibility to update oracle implementations easily. This includes:
spot jump oracle, all price feeds, (MTM cache)

todo: rename Allowlist

• make the interface be more general, can be a denyList instead of allowlist
• add comments on allowlist that this is an example not something we will launch with

todo: rename to OptionAsset

Make `ICashAsset` an `IAsset`

ICashAsset need to inherit IAsset, otherwise managers can't use ICashAsset and implicitly have it convertible to IAsset type

Ability for keep to accrue interest per account_id

In the cash asset, can we add a way to accrue interest to a specific user (update their balance) without a non-zero transfer? We'd like an easy way for bots to be able to update our users' accounts (otherwise we'd have to replicate the interest accruing logic in the orderbook).

I don't believe depositing or transferring amount=0 actually works because it does not accrue interest for zero transfers.

One possibility is to do a cash adjustment during this function call:

https://github.com/lyra-finance/v2-core/blob/c7e12ee712429c1908ee59bf23086109b9ec4ca0/src/assets/CashAsset.sol#LL235C2-L235C2

Update margining details

• use a "perp market price" oracle for margining perps
• unpaird margin in basic manager should use forward price instead of spot
• risk reducing trades

rename usdc price to stable rpice

Incorrect event emission

Description

In CashAsset::disableWithdrawFee(), the event WithdrawFeeDisabled is emitted even in case the exchangeRate is below 1e18 and the temporaryWithdrawFeeEnabled is not yet set to false.

Impact

Low: This can lead to incorrect assumptions in off-chain infrastructure due to incorrect events

Recommendation

Updating the code to only emit an event in case the if() is entered.

  /**
   * @notice Disable withdraw fee when the cash asset is back to being solvent
   */
  function disableWithdrawFee() external {
    uint exchangeRate = _getExchangeRate();
    if (exchangeRate >= 1e18 && temporaryWithdrawFeeEnabled) {
      temporaryWithdrawFeeEnabled = false;
      smFeePercentage = previousSmFeePercentage;
      emit WithdrawFeeDisabled(exchangeRate);
    }
  }

References

https://github.com/lyra-finance/v2-core/blob/master/src/assets/CashAsset.sol#L274

Minor: min deposit during negative interest balance

Vlad found a bit of a quirk in the "cash asset deposit bypass of margin" while testing:

if (
    assetDeltas.length == 1 && 
    assetDeltas[0].asset == cashAsset && 
    assetDeltas[0].delta >= 0
) return;

- say Alice holds a max leveraged box (-$80 cash balance, IM == 0, so max borrow)
- wait 14 days
- try to deposit $0 to Alice
- this will revert because the accrued interest ends up being included in assetDeltas[0].delta. 
 A $0 deposit will actually become a -$0.30, so the bypass is not kicking in

We don't really see it as a UX problem (unlikely to happen),
but flagging it in case it breaks something...