Add Migrator

[shahthepro]

If you made a contract change, make sure to complete the checklist below before merging it in master.

Contract change checklist:

• [ ] Code reviewed by 2 reviewers.
• [ ] Copy & paste code review security checklist below this checklist.
• [ ] Unit tests pass
• [ ] Slither tests pass with no warning
• [ ] Echidna tests pass if PR includes changes to OUSD contract (not automated, run manually on local)

[DanielVF]

Code Review - Migrator

Requirements

We wish to provide one transaction + approval of OGV and veOGV stakes to OGN and xOGN stakes.

This contract collects ogv to the user's address, and then burns it there using an approval. OGN is then sent to a new stake for the user's address and/or sent directly to the user.

A simple migrate method can just burn and transfers without touching the staking system.

Authentication

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

Ethereum

• [x] Contract does not send or receive Ethereum.
• [x] Contract has no payable methods.

Cryptographic code

No crypto

Gas problems

• [x] Contracts with for loops must have either:
  • [x] A way to remove items
  • [ ] Can be upgraded to get unstuck
  • [ ] Size can only controlled by admins
• [x] 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

• [x] Does not contain selfdestruct
• [x] 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.
• [x] Address.isContract should be treated as if could return anything at any time, because that's reality.

Overflow

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

Proxy

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

Events

• [x] All state changing functions emit events

Medium Checks

Rounding

• [x] Contract rounds in the protocols favor
• [x] Contract does not have bugs from loosing rounding precision
• [x] Code correctly multiplies before division
• [x] Contract does not have bugs from zero or near zero amounts

Dependencies

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

External calls

• [x] Contract addresses passed in are validated
• [x] No unsafe external calls
• [ ] Reentrancy Guards
  • [ ] guards on all state changing functions
    • [x] Still doesn't protect against external contracts changing the state of the world if they are called.
    • [x] All external contracts used are hardcoded and safe.
• [x] No malicious behaviors
• [x] Low level call() must require success.
• [x] No slippage attacks (we need to validate expected tokens received)
• [x] Oracles, one of:
  • [x] No oracles
  • [ ] Oracles can't be bent
  • [ ] If oracle can be bent, it won't hurt us.
• [x] 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
• [ ] If tests interact with AMM make sure enough edge cases (pool tilts) are tested. Ideally with fuzzing.

Deploy

Deploy file not yet written

Logic

• [x] Public methods only send funds to msg.sender
• [x] Contract only pulls funds from msg.sender
• [x] Contract does not hold any veOGV
• [x] All converted OGN is distributed to the user

Logic looks correct.

Deployment Considerations

No deploy yet

Internal State

• [x] End time can only be written once
• [x] End time is after start time
• [x] Once started, OGN balance will always be sufficient to migrate everyone, until migration over.

Attack

This code will hold and then distribute 40%ish of the circulating supply of OGN. An exploit could have major financial consequences for token holders.

Although OGV is the current governance token and xOGN will be the future governance token, this migration contract should not be able to directly cause a loss of governance, since during the migration period, a multisig will temporary control governance.

We will be topping off the code with excess OGN tokens, since we don't know precisely when the migration will start. If the deployer still had governance, they could skim off excess tokens. We plan to move tokens to this contract and start() it from the same governance transaction, which means the governance action would fail if the migration contract was not owned by governance.

A concern would be if a user could take OGV tokens from someone else. This does not appear to be possible since all funds pulls are from msg.sender.

Another concern would be getting too many tokens for a conversion. The conversion rate matches the governance proposal. We will have another sanity check in that the amount of OGN initially transferred to the contract will be checked. Finally, the isSolvent modifier should revert if we give out too many tokens.

Flavor

Flavor is good.

[DanielVF]

Code Review - veOGV

Requirements

• Instant unstakes
• Migrator controlled unstakes to the user
• All staking disabled

Authentication

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

Ethereum

• [x] Contract does not send or receive Ethereum.
• [x] Contract has no payable methods.

Cryptographic code

no crypto

Gas problems

• [x] Contracts with for loops must have either:
  • [ ] A way to remove items
  • [x] Size can be changed by user
  • [ ] Can be upgraded to get unstuck
  • [ ] Size can only controlled by admins
• [x] 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

• [x] Does not contain selfdestruct
• [x] 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.
• [x] Address.isContract should be treated as if could return anything at any time, because that's reality.

Overflow

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

Proxy

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

Events

• [x] All state changing functions emit events

Rounding

• [x] Contract rounds in the protocols favor
• [x] Contract does not have bugs from loosing rounding precision
• [x] Code correctly multiplies before division
• [x] Contract does not have bugs from zero or near zero amounts

Dependencies

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

External calls

• [x] Contract addresses passed in are validated
• [x] No unsafe external calls
• [x] Reentrancy
  • [ ] guards on all state changing functions
  • [x] Only interact with trusted contracts
• [x] No malicious behaviors
• [x] Low level call() must require success.
• [x] No slippage attacks (we need to validate expected tokens received)
• [x] Oracles, one of:
  • [x] No oracles
  • [ ] Oracles can't be bent
  • [ ] If oracle can be bent, it won't hurt us.
• [x] Do not call balanceOf for external contracts to determine what they will do when they use internal accounting

Deploy

No deploy code written

When deploy code is written, a collect call should be made just before this upgrade.

Logic

Logic appears correct.

Internal State

• [x] accRewardsPerShare no longer changes
• [x] No new lockups can be pushed

Attack

This contract holds 3.7 billion OGV, which will soon be convertible to OGN - 40 something million at today's prices.

An attack would attempt to gain access to another user's tokens, get more tokens than owed, or double withdraw.

• [x] All unstakes transfer OGV to the stake owner
• [x] The amount calculations to send to the user are a simple sum of staked amounts being cleared
• [x] Stakes are disabled in storage as soon as amounts are added to total
• [x] Duplicate stake ids in a removal should fail

Flavor

The code is slightly awkward because of the tension between keeping backwards compatibility versus most functionality no longer being needed. However there is not any anticipated future work on this contract, and it's okay as is.

[DanielVF]

Noting that we should do some fork testing of this during the audit.