Update Buyback contract for OGV & CVX swaps

[shahthepro]

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

Refer to our documentation for more details about contract security best practices.

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)

[github-actions]

	Warnings
⚠️	👀 This PR needs at least 2 reviewers

Generated by 🚫 dangerJS against 2d6fa59

[notion-workspace]

Update Buyback to support OETH

[codecov]

Codecov Report

Merging #1876 (2d6fa59) into master (3ae83eb) will increase coverage by 0.11%.
The diff coverage is 94.59%.

@@            Coverage Diff             @@
##           master    #1876      +/-   ##
==========================================
+ Coverage   67.60%   67.71%   +0.11%     
==========================================
  Files          50       52       +2     
  Lines        2710     2726      +16     
  Branches      700      706       +6     
==========================================
+ Hits         1832     1846      +14     
- Misses        875      877       +2     
  Partials        3        3              

Files	Coverage Δ
contracts/contracts/buyback/OETHBuyback.sol	85.71% <85.71%> (ø)
contracts/contracts/buyback/OUSDBuyback.sol	85.71% <85.71%> (ø)
contracts/contracts/buyback/BaseBuyback.sol	96.66% <96.66%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[DanielVF]

Is there a reason we use one contract with lots of if statements, rather than just deploying one simple contract twice? Seems like it would keep the code simpler, and with that be easier to change in the future?

The different swap routes for each coin could just be an immutable bytestring passed in during the constructor.

[DanielVF]

Thanks for cleaning up the now unused swap() method. 👍

[sparrowDom]

LGTM

[DanielVF]

Buybacks Review

Requirements

This PR is shifting to include buying and locking CVX as a part of buyback contract, rather than as a separate strategist action.

We're also now using a mostly shared codebase for both OETH and OUSD buybacks.

Deployment Considerations

Once we create the governance proposals, we'll need to not do buybacks on the migrated old buyback contracts, since the amounts transferred will be hardcoded into the governance action.

Internal State

Only config

Attack

The consequences of this contract going bad should be small. A week or two of a percentage of yield.

An attacker could try to get the contract to transfer tokens to themselves, however, all transfer destinations are controlled by governance.

The primary threat is MEV action during the swaps. This is defended against via minimum output parameters passed in by the strategist. These minimum output amounts are passed to the Uniswap router for checking.

Logic

Looks correct

Tests

Not checked

Flavor

Looking good

Overflow

• Never use "+" or "-", always use safe math or have contract compile in solidity version > 0.8
• Check that all for loops use uint256

Proxy

• Make sure proxy implementation contracts don't initialize variable state on variable declaration and do it rather in initialize function.

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.

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.

Deploy

• Check that any deployer permissions are removed after deploy

Authentication

• Never use tx.origin
• Check that every external/public function should actually be external
• Check that every external/public function has the correct authentication

Cryptographic code

• Contracts that roll their own crypto are terrifying
• Note that a failed signature check will result in a 0x00 result. Make sure that the result throws if it returns this.
• Beware of signed data being used in a replay attack to other contracts.

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.

External calls

• Contract addresses from public in are validated
• Unsafe external calls
• Reentrancy guards on all publicly accessable state changing functions
  • Still doesn't protect against external contracts changing the state of the world if they are called.
• Malicious behaviors
• Could fail from stack depth problems (low level calls 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.
• Don't call balanceOf for external contracts to determine what they will do, when they instead use internal accounting?

Ethereum

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

[DanielVF]

Fork tests:

from world import *
OETH_WHALE = '0xEADB3840596cabF312F2bC88A4Bb0b93A4E1FF5F'
NEW_OETH_BUYBACK = '0x85094b52754591A3dE0002AD97F433584389aea0'
buyback_oeth = Contract.from_abi('buyback_oeth', NEW_OETH_BUYBACK, buyback.abi, owner=STRATEGIST)
buyback_ousd = buyback

buyback.swap(100*1e18, 1e18, 1e18, {'from': STRATEGIST})
show_transfers(history[-1])
buyback.swap(100*1e18, 1e18, 20* 1e18, {'from': STRATEGIST})
buyback.swap(100*1e18, 99*1e18, 1* 1e18, {'from': STRATEGIST})
buyback.swap(100*1e18, 999*1e18, 1* 1e18, {'from': STRATEGIST})
buyback.swap(100*1e18, 9999*1e18, 1* 1e18, {'from': STRATEGIST})
buyback.swap(100*1e18, 99999*1e18, 1* 1e18, {'from': STRATEGIST})


oeth.transfer(buyback_oeth, 10*1e18, {'from': OETH_WHALE})
buyback_oeth.swap(1e18, 1e18, 1e18, {'from': STRATEGIST})
buyback_oeth.swap(int(1e18)+1, 1e18, 1e18, {'from': STRATEGIST})
show_transfers(history[-1])

[DanielVF]

Three or four minor tweaks needed, but otherwise looks good. 👍

[DanielVF]

Should be good for deploy.