Simple harvester for OETH #2333

clement-ux · 2024-12-23T12:44:02Z

Simple Harvester

This PR aims to deploy a simple harvester for the OETH AMO to replace the current.
This new harvester is simpler, as it will only harvest reward and send it to the strategist.

Note: strategist and operator addresses are set to the deployer address but must be changed.

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

Deploy checklist

Two reviewers complete the following checklist:

- [ ] All deployed contracts are listed in the deploy PR's description
- [ ] Deployed contract's verified code (and all dependencies) match the code in master
- [ ] Contract constructors have correct arguments
- [ ] The transactions that interacted with the newly deployed contract match the deploy script.
- [ ] Governance proposal matches the deploy script
- [ ] Smoke tests pass after fork test execution of the governance proposal

github-actions · 2024-12-23T12:44:34Z

	Warnings
⚠️	👀 This PR needs at least 2 reviewers

Generated by 🚫 dangerJS against f0559ad

codecov · 2024-12-23T19:18:44Z

Codecov Report

Attention: Patch coverage is 0% with 25 lines in your changes missing coverage. Please review.

Project coverage is 51.84%. Comparing base (3d7c282) to head (f0559ad).
Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
...ontracts/contracts/harvest/OETHHarvesterSimple.sol	0.00%	25 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2333      +/-   ##
==========================================
- Coverage   52.15%   51.84%   -0.32%     
==========================================
  Files          80       81       +1     
  Lines        4101     4126      +25     
  Branches     1079     1084       +5     
==========================================
  Hits         2139     2139              
- Misses       1959     1984      +25     
  Partials        3        3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

into clement/simple-harvester

notion-workspace · 2024-12-30T09:22:45Z

Update Harvester

contracts/contracts/harvest/OETHHarvesterSimple.sol

DanielVF · 2025-01-02T18:27:59Z

Code review - Simple harvester

Requirements

We want to be able to route some strategy reward token earnings directly to the strategist, for further allocation/distribution.

This contract allows anyone to harvest, does not pay incentives, and routes all to the strategist. The strategist can control which contracts this can call, while governance controls which strategies allow this contract to call it.

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
Every use of msg.sender is correct

Ethereum

Contract does not send or receive Ethereum.
Contract has no payable methods.
Contract does not have a vulnerablity from being sent self destruct ETH

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
- Size can be controlled by caller
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

no magic

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
- 🟠 constructor not setting an event for strategist. Noted in code.
  - Fixed

Rounding and casts

No rounding

Dependencies

No dependancies outside our code.

External calls

Contract addresses passed in by users are validated
No unsafe external calls
No slippage attacks (we need to validate expected tokens received)
- Tokens amounts cannot be manipulated
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

Deploy

Deployer permissions are removed after deploy

Logic

Logic looks correct

Internal State

No internal state, config only

Attack

This contract will move weekly/daily reward token yield. It will not touch user funds.

If an attacker could change the strategist address, they would receive the yield. However, the strategist address can only be controlled by the governor. The governor can only be changed by the governor.

If the attacker were able to whitelist a malicious strategy this would not allow taking yield, since yield only goes to the strategist, and all coins are emptied after each strategy harvest runs. This could be used as a “clever backdoor” for recovering random coins sent to this contract by accident.

A strategy that fails to be harvested can just be not called on, so that it does not block other harvests.

Flavor

Flavor is good.

Functions follow validate, read, compute, write, call, event, validate

contracts/deploy/mainnet/114_simple_harvester.js

contracts/contracts/harvest/OETHHarvesterSimple.sol

sparrowDom · 2025-01-03T17:17:34Z

contracts/test/harvest/simple-harvester.mainnet.fork-test.js

+    expect(await simpleOETHHarvester.governor()).to.be.equal(
+      addresses.mainnet.Timelock
+    );
+    expect(await simpleOETHHarvester.strategist()).to.be.equal(deployerAddr);


nit: this line will change as well once strategist address gets updated

sparrowDom · 2025-01-03T17:19:13Z

contracts/test/harvest/simple-harvester.mainnet.fork-test.js

+      )
+    ).to.be.equal(false);
+    await simpleOETHHarvester
+      .connect(timelock)


we should also have a test for the strategist to set a supported strategy

sparrowDom · 2025-01-03T17:38:20Z

Requirements

This PR introduces a simplified Harvester which harvests the reward tokens from strategies and without having the ability to swap sends them to the strategist. The strategist has the permissions to pick a set of strategies to be harvested. The governor sets the strategist and approves the harvester on the strategy contract.

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.
Contract has no payable methods.
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: (no concerning loops)
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

no dependencies

External calls

Tests

Each publicly callable method has a test
Each logical branch has a test
can also call that function
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 looks ok

Thinking

Logic

Logic looks ok

Deployment Considerations

Aside from changing the strategist address deployment is ok

Internal State

Internal state is just config

Attack

This contract doesn't touch user funds, rather the strategy rewards. In worst case if the strategist is compromised then:

all the existing yield up to the compromise event and the yield accrued in the governance procedure (governance proposal duration + timelock (2 day timelock)) could be stolen.
Not related to the contract code, but any funds held by the strategist would also be stolen in such case.

Flavor

code is simple

clement-ux · 2025-01-04T21:33:19Z

🔴 Deployer permissions are removed after deploy

currently the deployer address is still set as the strategist

@sparrowDom Which address should I use for the strategist at the deployment?

shahthepro · 2025-01-05T08:24:53Z

contracts/contracts/harvest/OETHHarvesterSimple.sol

+            if (balance > 0) {
+                // Transfer to strategist
+                IERC20(rewardTokens[i]).safeTransfer(strategistAddr, balance);
+                emit Harvested(rewardTokens[i], balance);


nit: Let's emit the strategy address here as well?

Yes good idea, done in this commit: 0e8ab277

sparrowDom · 2025-01-06T11:50:41Z

@clement-ux you can set it to the mainnet Strategist: https://github.com/OriginProtocol/origin-dollar/blob/master/contracts/hardhat.config.js#L47

into clement/simple-harvester

DanielVF · 2025-01-06T12:41:06Z

@sparrowDom I don't think we need to worry about gas on the coin loop. The number of coins is not under control, and we can upgrade the strategy if for some reason we had the hundreds of reward tokens it would take to hit gas limits.

sparrowDom · 2025-01-06T13:42:48Z

contracts/test/harvest/simple-harvester.mainnet.fork-test.js


    expect(await simpleOETHHarvester.governor()).to.be.equal(
      addresses.mainnet.Timelock
    );
+
+    console.log("strategistAddr: ", strategistAddr);


you can probably delete this one

Good catch, indeed 😅

sparrowDom

@DanielVF cool sounds good.
@clement-ux all the things from my full report have been addressed. I've updated the report. Great job on this one 💪

clement-ux added 6 commits December 20, 2024 15:43

feat: Implement simple harvester.

8fe5bee

feat: support multiple strategies.

e14774a

fix: adjust error message.

16513f4

feat: add test for simple harvester.

128c288

feat: WIP add deployment script for simple harvester.

cb78145

fix: adjust deployment for simple harvester.

0208ddd

doc: add comment on deployment file.

fe7596c

Merge branch 'master' of https://github.com/OriginProtocol/origin-dollar

20bab54

into clement/simple-harvester

clement-ux changed the title ~~Clement/simple harvester~~ Simple harvester for OETH Dec 30, 2024

clement-ux marked this pull request as ready for review December 30, 2024 09:17

clement-ux requested review from sparrowDom, DanielVF, naddison36 and shahthepro as code owners December 30, 2024 09:17

OriginProtocol deleted a comment from notion-workspace bot Dec 30, 2024

clement-ux added 2 commits December 30, 2024 10:29

prettier deployment file.

8142811

add prettier-ignore.

0f22981

DanielVF reviewed Dec 30, 2024

View reviewed changes

contracts/contracts/harvest/OETHHarvesterSimple.sol Outdated Show resolved Hide resolved

sparrowDom reviewed Dec 30, 2024

View reviewed changes

contracts/contracts/harvest/OETHHarvesterSimple.sol Outdated Show resolved Hide resolved

DanielVF reviewed Dec 30, 2024

View reviewed changes

contracts/contracts/harvest/OETHHarvesterSimple.sol Outdated Show resolved Hide resolved

sparrowDom reviewed Dec 30, 2024

View reviewed changes

contracts/contracts/harvest/OETHHarvesterSimple.sol Outdated Show resolved Hide resolved

DanielVF reviewed Dec 30, 2024

View reviewed changes

contracts/contracts/harvest/OETHHarvesterSimple.sol Outdated Show resolved Hide resolved

sparrowDom reviewed Dec 30, 2024

View reviewed changes

contracts/contracts/harvest/OETHHarvesterSimple.sol Show resolved Hide resolved

fix: use changeGov instead of setGov in constructor.

534ce82

DanielVF previously approved these changes Jan 2, 2025

View reviewed changes

DanielVF reviewed Jan 2, 2025

View reviewed changes

contracts/deploy/mainnet/114_simple_harvester.js Outdated Show resolved Hide resolved

sparrowDom reviewed Jan 3, 2025

View reviewed changes

contracts/contracts/harvest/OETHHarvesterSimple.sol Outdated Show resolved Hide resolved

sparrowDom reviewed Jan 3, 2025

View reviewed changes

contracts/contracts/harvest/OETHHarvesterSimple.sol Outdated Show resolved Hide resolved

sparrowDom reviewed Jan 3, 2025

View reviewed changes

contracts/contracts/harvest/OETHHarvesterSimple.sol Show resolved Hide resolved

sparrowDom reviewed Jan 3, 2025

View reviewed changes

feat: use Strategizable instead of Governable.

639e7ae

clement-ux dismissed DanielVF’s stale review via 639e7ae January 4, 2025 21:27

feat: add rescue token.

afaf9e5

test: add more tests.

9eefde2

shahthepro reviewed Jan 5, 2025

View reviewed changes

feat: add strategy address in event.

0e8ab27

clement-ux added 2 commits January 6, 2025 12:55

Merge branch 'master' of https://github.com/OriginProtocol/origin-dollar

8d2de44

into clement/simple-harvester

fix: use correct address for strategist at deployment.

543378d

sparrowDom reviewed Jan 6, 2025

View reviewed changes

sparrowDom previously approved these changes Jan 6, 2025

View reviewed changes

fix: remove console.log

f0559ad

clement-ux dismissed sparrowDom’s stale review via f0559ad January 6, 2025 13:52

sparrowDom approved these changes Jan 8, 2025

View reviewed changes

shahthepro approved these changes Jan 8, 2025

View reviewed changes

shahthepro merged commit a29405f into master Jan 8, 2025
14 of 18 checks passed

shahthepro deleted the clement/simple-harvester branch January 8, 2025 08:49

clement-ux mentioned this pull request Jan 8, 2025

Deployed 114 - Simple OETH Harvester. #2344

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simple harvester for OETH #2333

Simple harvester for OETH #2333

clement-ux commented Dec 23, 2024 •

edited

Loading

github-actions bot commented Dec 23, 2024 •

edited

Loading

codecov bot commented Dec 23, 2024 •

edited

Loading

notion-workspace bot commented Dec 30, 2024

DanielVF commented Jan 2, 2025

sparrowDom Jan 3, 2025

sparrowDom Jan 3, 2025

sparrowDom commented Jan 3, 2025 •

edited

Loading

clement-ux commented Jan 4, 2025

shahthepro Jan 5, 2025

clement-ux Jan 5, 2025

sparrowDom commented Jan 6, 2025

DanielVF commented Jan 6, 2025

sparrowDom Jan 6, 2025

clement-ux Jan 6, 2025

sparrowDom left a comment

Simple harvester for OETH #2333

Simple harvester for OETH #2333

Conversation

clement-ux commented Dec 23, 2024 • edited Loading

Simple Harvester

Code Change Checklist

Deploy checklist

github-actions bot commented Dec 23, 2024 • edited Loading

codecov bot commented Dec 23, 2024 • edited Loading

Codecov Report

notion-workspace bot commented Dec 30, 2024

DanielVF commented Jan 2, 2025

Code review - Simple harvester

Requirements

Easy Checks

Authentication

Ethereum

Cryptographic code

Gas problems

Black magic

Overflow

Proxy

Events

Rounding and casts

Dependencies

External calls

Deploy

Logic

Internal State

Attack

Flavor

sparrowDom Jan 3, 2025

Choose a reason for hiding this comment

sparrowDom Jan 3, 2025

Choose a reason for hiding this comment

sparrowDom commented Jan 3, 2025 • edited Loading

Requirements

Easy Checks

Authentication

Ethereum

Cryptographic code

Gas problems

Black magic

Overflow

Proxy

Events

Medium Checks

Rounding and casts

Dependencies

External calls

Tests

Deploy

Thinking

Logic

Deployment Considerations

Internal State

Attack

Flavor

clement-ux commented Jan 4, 2025

shahthepro Jan 5, 2025

Choose a reason for hiding this comment

clement-ux Jan 5, 2025

Choose a reason for hiding this comment

sparrowDom commented Jan 6, 2025

DanielVF commented Jan 6, 2025

sparrowDom Jan 6, 2025

Choose a reason for hiding this comment

clement-ux Jan 6, 2025

Choose a reason for hiding this comment

sparrowDom left a comment

Choose a reason for hiding this comment

clement-ux commented Dec 23, 2024 •

edited

Loading

github-actions bot commented Dec 23, 2024 •

edited

Loading

codecov bot commented Dec 23, 2024 •

edited

Loading

sparrowDom commented Jan 3, 2025 •

edited

Loading