Sonic Staking

[naddison36]

Dependencies

• OZ Audit changes for OETH Withdrawal Queue #2168
• Yield Delegation / Rebasing Token Rewrite #2298
• Fix Base unit and fork tests #2345
• Generated new Origin token diagrams #2349
• Sonic Repo Setup #2348

Contracts

Sonic Staking Strategy

Tests

Sonic Unit Tests

The Sonic unit tests are in contracts/test/vault/os-vault.sonic.js.

yarn test:sonic

Sonic Fork Tests

The Sonic fork tests are in:

• contracts/test/vault/vault.sonic.fork-test.js
• contracts/test/strategies/sonicStaking.sonic.fork-test.js
• contracts/test/zapper/osonic-zapper.sonic.fork-test.js

# start a local Sonic forked node
yarn run node:sonic

# In another terminal, run the Sonic fork tests
yarn test:sonic-fork

Deployment

The Sonic deployment files for launch are

• contracts/deploy/sonic/001_vault_and_token.js
• contracts/deploy/sonic/002_oracle_router.js
• contracts/deploy/sonic/003_sonic_staking_strategy.js

To run the deployment to Sonic

yarn run deploy:sonic

When deploying to the Tenderly Testnet, change SONIC_PROVIDER_URL in your .env file to point to the Testnet.

Code Change Checklist

To be completed before internal review begins:

• The contract code is complete
• Executable deployment file
• Fork tests that test after the deployment file runs
• Unit tests *if needed
• The owner has done a full checklist review of the code + tests

Internal review:

• Two approvals by internal reviewers

[sparrowDom]

At some point we should probably override the constructor so it doesn't accept the Token Name & Symbol just to avoid confusion

[sparrowDom]

Requirements

PR supports delegation of Wrapped Sonic to validators to earn staking rewards.

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

Sonic token

• [🟡] Contract does not send or receive S
  • It does receive S but it does not affect its checkBalance or behaviour of other functions. The withdrawFromSFC and collectRewards check only for the increment of S ignoring previous states. The withdrawAll does check for the whole balance of S and withdraws all. Donating S to contract only changes the beaviour where withdrawAll is able to withdraw more than was "rightfully" earned.
  • hijacking transaction execution: contracts makes sure that any storage altering operations happen before it is calling external SFC contract which can trigger transfer of S token.
• [🟡] Contract has no payable methods.
  • It does but limits the contracts that can send S to it. Though with WrappedSonic's withdrawTo this doesn't really prevent receiving S. Anyone can wrap S into WS and withdraw it to our staking contract via the WS contract.
• Contract is not vulnerable to being sent self destruct S.
  • with exception of withdrawAll the contract ignores any existing S that has been received. It is able to recover all S and accounting is not affected by having received S.

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

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

No Rounding

Dependencies

• Review any new contract dependencies thoroughly (e.g. OpenZeppelin imports) when new dependencies are added or version of dependencies changes.

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

Tests

• Each publicly callable method has a test
• Each logical branch has a test
• Each require() has a test
• Edge conditions are tested

Deploy

• Deployer permissions are removed after deploy

Strategy Specific

Strategy checks

• Check balance cannot be manipulated up AND down by an attacker
• No read only reentrancy on downstream protocols during checkBalance
• All reward tokens are collected
• The harvester can sell all reward tokens
  • The rewards are in S token so there is no need to swap
• 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() can always withdraw an amount equal to or larger than checkBalances report, even in spite of attacker manipulation.
  • yes considering there have been no slashing of the validators. Also full withdrawal is not immediate as it takes time to undelegate and withdraw
• WithdrawAll() cannot be MEV'd
• Strategist cannot steal funds

Downstream

Monitoring Ticket

• we should monitor validator slashings
• we should monitor SFC contract upgrades

Thinking

Logic

The logic looks sound

Deployment Considerations

Just a normal deploy governance process

Internal State

• checkBalance will return higher amount without any deposits / withdrawals each epoch (~10 minutes) as rewards are not accounted for separately
• withdrawAll is the only function picking up "unaccounted S tokens". Any other combination of Depositing / delegating / undelegating / Withdrawing / restakeRewards / CollectRewards should not change the total vault value. In case someone donates S to Sonic staking strategy and withdrawAll is called, then the vaultValue should increase
• defaultValidatorId should be one of supportedValidators
• nextWithdrawId should increase by 1

Attack

If a validator is slashed we could lose user funds. We will be monitoring validators and SFC contract upgrades to detect any anomalies.

A hijacked defender action relayer or strategist could undelegate all funds and prevent yield earning for 14 days. There is no way to prevent this in code.

Flavor

The code is a cleanly implementing the interface between the vault and the SFC contract.