ARM integration to Lending Markets

[naddison36]

Design Specification

The following spec is copied from https://www.notion.so/originprotocol/ARM-Lending-Markets-Design-1d684d46f53c805984b3f9b4c1cbadc8

The design decisions can be exported on request https://www.notion.so/originprotocol/ARM-Lending-Markets-Design-1d684d46f53c805984b3f9b4c1cbadc8?p=1cf84d46f53c80b8ad3ef07f6a45dd12&pm=s

Features

• An ARM maintains a liquidity buffer in the ARM to buy LSTs at a discount. Excess liquidity is deposited into the active lending market to earn yield.
• There can be many support lending markets but only one can be used at any point in time.
• The ARM will integrate to the lending markets using ERC-4626.

ARM Lending Market Use Cases

• Any can allocate liquidity in the ARM. This tries to adjust the ARM’s liquidity to match the ARM’s liquidity buffer.
  • This requires an active lending market to be set. A lending market with a zero address will disable the lending market feature.
  • If not enough liquidity in the ARM
    • and there is enough liquidity in the active lending market
      • liquidity is withdrawn from the active lending market.
    • and there is not enough enough liquidity in the active lending market
      • all liquidity is redeemed from the active lending market.
  • If too much liquidity in the ARM, liquidity is deposited to the active lending market.
• The Owner (Timelock) can add a supported lending market
  • The ARM’s liquidity asset must match the asset of the lending market’s vault
• The Owner (Timelock) can remove a supported lending market
  • Must not be the active lending market
• The Operator (Defender Relayer) can switch the active lending market
  • The lending market has to be one of the ARM’s supported lending markets
  • All liquidity is withdrawn from the previous active lending market.
    • If not all the liquidity can be removed as the market’s utilization is high, the transaction will fail
  • Liquidity is deposited to the new active lending market via the allocate process
• The Owner (Timelock) can set the ARM liquidity buffer

Harvester Use Cases

The Harvesters on Sonic will use the Magpie aggregator to get best execution of swapping Silo for wS.

Some key design decisions

• Anyone can collect token rewards from the supported lending market strategies. For the Silo lending markets, that will be the Silo token. Rewards from multiple lending market strategies can be collected before being swapped to the ARM’s associated liquidity asset.
• Only the Owner or Operator can swap token rewards for the associated ARM’s liquidity asset. For the Silo lending markets, that’s swapping Silo for wrapped S (wS). The initial implementation will only support the Magpie DEX aggregator. The Owner or Operator is responsible for calling the Magpie APIs and passing the the Magpie specific swap data.
• The Harvester will do the following validators of swaps
  • Swaps are to the associated ARM’s liquidity asset. ie wS
  • The swap price is within an allowed slippage from the Oracle price
  • The receiver of the swap is the Harvester’s rewards recipient. Initially that will be the ARM contract. That could be changed to a Dripper in the future.
• There will be a separate Harvester contract for each ARM. This is a lot simplifier than having a single Harvester that has to account for the incoming rewards tokens and sending the appropriate liquidity assets to each ARM.

Contracts

• Change license from MIT to BUSL-1.1
• Upgraded AbstractARM to park idle liquidity in a lending market
• Added OriginARM that implements AbstractARM by integrating to an Origin Vault
• Added SiloMarket contract that can harvest Silo rewards from a Silo lending market
• Added Harvester that can collect and swap Silo rewards to wS using the Magpie DEX aggregator
• Added ZapperARM that integrates to multiple ARMs by swapping S to wS.

Testing

To run all the tests

make test

To run the OriginARM fork tests

make test-c-Fork_Concrete_OriginARM

Deployment

The deployment scripts are
*script/deploy/sonic/001_DeployOriginARMProxy.sol
*script/deploy/sonic/002_DeployOriginARM.sol

In the .env file, set the DEPLOYER_PRIVATE_KEY and SONIC_URL variables

make deploy-sonic

[naddison36]

Requirements

See the PR description or Notion https://www.notion.so/originprotocol/ARM-Lending-Markets-Design-1d684d46f53c805984b3f9b4c1cbadc8

Easy Checks

Authentication

• Never use tx.origin
• Every external/public function is supposed to be externally accessible
• Every external/public function has the correct authentication

Ethereum

• Contract does not send or receive Ethereum.
  ZapperARM receives native S but that's ok.
• Contract has no payable methods.
  deposit on ZapperARM is payable but that's ok.
• Contract is not vulnerable to being sent self destruct ETH

Cryptographic code

• This contract code does not roll it's own crypto.
• No signature checks without reverting on a 0x00 result.
• No signed data could be used in a replay attack, on our contract or others.

Gas problems

• Contracts with for loops must have either:
  • A way to remove items
  • Can be upgraded to get unstuck
  • Size can only controlled by admins
• Contracts with for loops must not allow end users to add unlimited items to a loop that is used by others or admins.

Black magic

• Does not contain selfdestruct
• Does not use delegatecall outside of proxying. If an implementation contract were to call delegatecall under attacker control, it could call selfdestruct the implementation contract, leading to calls through the proxy silently succeeding, even though they were failing.
  Only the existing Proxy contract uses delegatecall
• Address.isContract should be treated as if could return anything at any time, because that's reality.

Overflow

• Code is solidity version >= 0.8.0
• All for loops use uint256

License

• The contract uses the appropriate limited BUSL-1.1 (Business) or the open MIT license
• If the contract license changes from MIT to BUSL-1.1 any contracts importing it need to also have their license set to BUSL-1.1

Proxy

• No storage variable initialized at definition when contract used as a proxy implementation.

Events

• All state changing functions emit events

Medium Checks

Rounding and casts

• Contract rounds in the protocols favor
• Contract does not have bugs from loosing rounding precision
• Code correctly multiplies before division
• Contract does not have bugs from zero or near zero amounts
• Safecast is aways used when casting

Dependencies

• Review any new contract dependencies thoroughly (e.g. OpenZeppelin imports) when new dependencies are added or version of dependencies changes.
• If OpenZeppelin ACL roles are use review & enumerate all of them.
• Check OpenZeppelin security vulnerabilities and see if any apply to current PR considering the version of OpenZeppelin contract used.

External calls

• Contract addresses passed in are validated
• No unsafe external calls
• Reentrancy guards on all state changing functions
  • Still doesn't protect against external contracts changing the state of the world if they are called.
• No malicious behaviors
• Low level call() must require success.
• No slippage attacks (we need to validate expected tokens received)
  The Harvester will not have slippage protection until there is a Silo Oracle on Sonic
• Oracles, one of:
  • No oracles
  • Oracles can't be bent
  • If oracle can be bent, it won't hurt us.
• Do not call balanceOf for external contracts to determine what they will do when they use internal accounting
  AbstractARM uses balanceOf in the new allocate and setActiveMarket functions.

Tests

• Each publicly callable method has a test
• Each logical branch has a test
• Each require() has a test
• Edge conditions are tested
• If tests interact with AMM make sure enough edge cases (pool tilts) are tested. Ideally with fuzzing.

Deploy

• Deployer permissions are removed after deploy

Downstream

• We have monitoring on all backend protocol's governances
• We have monitoring on a pauses in all downstream systems

Thinking

Logic

• Correct usage of global & local variables. -> they might differentiate only by an underscore that can be overlooked (e.g. address vs _address).

Deployment Considerations

• The final set of Silo Markets is still to be decided on
• The currently is no Oracle for Silo on Sonic.

Internal State

• What can be always said about relationships between stored state
• What must hold true about state before a function can run correctly (preconditions)
• What must hold true about the return or any changes to state after a function has run.

Does this code do that?

Attack

For the initial deployment, the Harvester will not have an Oracle for the Silo rewards. This means the Operator can route Silo to wS swaps to less liquid markets.

Flavor

The lending market integration was made a lot simpler by only having one active lending market. If there is a large amount of idle liquidity deposited into that active market, then that can diminish the returns as the utilization of the market will drop. It could be more profitable to split the idle liquidity across multiple lending markets but that adds a lot of complexity that isn't needed for now.

[pandadefi]

This block can be skipped if activeMarket is not set.

[naddison36]

Changed in cc88ff5

[pandadefi]

In case a withdrawal request is transferred to the OriginARM contract, you won't be able to claim it because this line will underflow.

This issue also exists in the lido contract.

arm-oeth/src/contracts/LidoARM.sol

Line 155 in 2e4cffa

lidoWithdrawalQueueAmount -= totalAmountRequested;

A simple fix could be

if (vaultWithdrawalAmount <  amountClaimed) {
vaultWithdrawalAmount = 0;
}

[naddison36]

Good spot for claiming the Lido withdrawals. I've fixed the LidoARM with bb0b2cd

It won't be an issue for the Origin ARM as withdrawals from the Origin Vault are not transferrable. I'll put in a comment.

[DanielVF]

🟠 The intention of the conditional in this function is that totalAssets() never returns a number lower than MIN_TOTAL_SUPPLY.

However, this function can return a number lower than MIN_TOTAL_SUPPLY, including returning a zero. The current code results in a discontinuity as the fee approaches and then passes newAvailableAssets. The returned assets goes down to zero and then jumps back up to the minimum total supply.

What we actually want here is a minimum function, taking the larger of two values, while also avoiding any subtraction before we know that the subtraction is safe. I think the correct if statement here might be this:

if (fees + MIN_TOTAL_SUPPLY >= newAvailableAssets) return MIN_TOTAL_SUPPLY;

[naddison36]

Excellent spot. Fixed in 5b1a9cf

[DanielVF]

OETH ARM

Requirements

This PR is adding the ability to auto-allocate liquidity to a 4626 lending market to earn extra yield on idle capital.

It does this via an intermediate wrapper 4626 contract that provides the ability to harvest rewards.

One open question is if anyone should be able to move liquidity back into the contract, or if it needs to be operator only so that the prices can be set before liquidity is re-added.

A portion of our volume is very large transactions. It would be helpful to know how many of these transactions are actually users wanting to transaction large amounts which is good or are bots looking the transaction large amounts which we don't want we would prefer to change our prices and raise them. Knowing this can help us determine how big of an allocation buffer we should set.

Having this lending platform option puts a threshold on how far we should change the prices. If we're going to be making 2% on a lending platform then we shouldn't set our sell price margins thin enough to the point where we will make less than the 2% going through a redeem cycle.

Authentication

• Never use tx.origin
• Every external/public function is supposed to be externally accessible
• Every external/public function has the correct authentication

Ethereum

🟣 Ethereum is only accepted for payable receives to receive WETH and for Zap contracts.

Cryptographic code

_ No crypto._

Gas problems

• Contracts with for loops must have either:
  • A way to remove items
  • Can be upgraded to get unstuck
  • Size can only controlled by admins
• Contracts with for loops must not allow end users to add unlimited items to a loop that is used by others or admins.

Black magic

• Does not contain selfdestruct
• Does not use delegatecall outside of proxying. If an implementation contract were to call delegatecall under attacker control, it could call selfdestruct the implementation contract, leading to calls through the proxy silently succeeding, even though they were failing.
• Address.isContract should be treated as if could return anything at any time, because that's reality.

Overflow

• Code is solidity version >= 0.8.0
• All for loops use uint256

License

• The contract uses the appropriate limited BUSL-1.1 (Business) or the open MIT license
• If the contract license changes from MIT to BUSL-1.1 any contracts importing it need to also have their license set to BUSL-1.1

Proxy

• No storage variable initialized at definition when contract used as a proxy implementation.

Events

• All state changing functions emit events
  • AMM swaps do not, and we are okay with that.

Rounding and casts

• Contract rounds in the protocols favor
• Contract does not have bugs from loosing rounding precision
• Code correctly multiplies before division
• Contract does not have bugs from zero or near zero amounts
• Safecast is aways used when casting

Dependencies

No dependency changes

External calls

• Contract addresses passed in are validated
• No unsafe external calls
• 🟠✅ No slippage attacks (we need to validate expected tokens received) Fixed

🟣 Harvesting requires correct minimum amounts to be passed in in order to avoid sandwich attacks. This is okay since this is not user funds.

Deploy

• Deployer permissions are removed after deploy

Strategy Specific

Strategy checks

• Check balance cannot be manipulated up AND down by an attacker
  • As long as the 4626 lending market is well behaved.
  • The operator could reduce the active assets by the size of the minimum redeem amount. See discussion under attacks
• No read only reentrancy on downstream protocols during checkBalance
  • A read-only reentrancy in a downstream 4626 could result in a catastrophic loss of user funds. We must ensure that 4626 contracts used as markets are not subject to read-only reentry.
• All reward tokens are collected
• The harvester can sell all reward tokens
• No funds are left in the contract that should not be as a result of depositing or withdrawing
• All funds can be recovered from the strategy by some combination of depositAll, withdraw, or withdrawAll()
• WithdrawAll() cannot be MEV'd

Logic

Appears correct, outside of panda’s finding on extra withdraws

Attack

This new code holds our user’s funds. It also tracks how much funds we think we have. Bugs in this new could result in a total loss of user funds.

The harvester code could only result in a loss of some of our yield. Because these these stakes are much, much, much smaller. I'm going to spend less time examining them.

We want to ensure that our balances stay within a rounding error when moving funds into or out of the lending markets.

We need to make sure that any funds that go into a lending market can only come back to us.

The asset balance accounting should be correct and not subject to manipulation.

The intersection between withdrawals and balance checking should be correct.

What could the impacts of code failure in this code be.

What conditions could cause this code to fail if they were not true.

Does this code successfully block all attacks.

🟠✅ An operator can artificially reduce the supply of the token by making a small small amount of tokens fall under the minimum mint required when switching active strategies.

This minimum value is currently set to 1e7. Assuming a total supply of 1e22, this would give an attack a 1/1e15 privileged mint. Assuming an attacker was using a 100 million dollars, this would give the attacker a free $0.0000001 per loop. Current sonic gas prices are more than $0.01 for something basic. This is currently attack economically unfeasible.

This has been fixed.

Flavor

See comment on read only reentrancy check on silo.

[naddison36]

@DanielVF looking at example deposit and allocate txs for read-only reentrancy, the calls are contained within the Silo contracts.
There are a lot of calls from the Varlamore S Vault as it loops over the different markets and calls the various Silo lending market contracts. But none of them call out to third party contracts. eg token contracts.
There are also no native S transfers in the Silo calls.

Deposit

Allocate