Add OGNRewardsSource contract #408

shahthepro · 2024-04-17T12:01:41Z

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)

contracts/OGNRewardsSource.sol

contracts/FixedRateRewardsSource.sol

pandadefi · 2024-04-24T10:44:14Z

@shahthepro I have added my comments regarding the changes.

* Add Migrator contract * Fix some tests * Code review changes * Update OgvStaking tests * Disable delegation tests * Allow just unstakes * Fix comment * More cleanup * Fix brownie tests

contracts/FixedRateRewardsSource.sol

* Check available balance in `previewRewards` * Chore: forge fmt --------- Co-authored-by: Daniel Von Fange <daniel@leancoder.com>

DanielVF · 2024-05-09T20:37:04Z

FixedRateRewardsSource

Requirements

The FixedRateRewardsSource works to provide a steady per block stream of incentives funds to the xOGN staking contract.

Rewards can be sent to the contract from any source (including incentives, as well as protocol buybacks), but will drip out at a rate that can be changed by the strategist.

In the event that not enough funds are in the dripper, what funds are there are given out, and the dripper is reset to start accruing max dripped funds again. In this way even if there are fewer funds than expected coming in, the contract will still hand out whatever funds it does get.

Deployment Considerations

There's a potential circular address dependency between this contract and the staking contract. Both need to know the address of the other. We've taken the simple route and made this contract us a mutable address for the staking contract, while the staking contract uses an immutable address for this.

If the contract is not initialized, collect rewards will revert because the message sender is not 0, preventing anything weird from happening.

This should be deployed into a proxy.

Internal State

config.lastCollected should never move downwards. This holds. It is always set to the current timestamp, which does not move down.

config.lastCollected should be set any time rewards are sent from the contract. It is.

We should not have resolution issues on config.ratePerSecond, since we can pay at a rate down to trillions of a penny's worth of yield per day.

Attack

Depending on how this contract is used, it could hold up to a month or two of incentives. This could be a substantial amount reward tokens. This contract does not hold any user funds.

Attackers could attempt to steal funds. However, Funds only transfer to the admin controlled rewards target.

Attackers could attempt to increase the number of rewards paid. However rewards paid can only be increased by donating to the contract (and only then if it does not have enough rewards)

A collectRewards should be called before changing the setRewardsPerSecond, if we care about accuracy, or if it has not been called in a long time. This ensures that the past is paid at the past rate.

Logic

Contract logic looks correct .

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.

Flavor

There are a few tiny flavor changes I see, but we can leave the contract as is.

In setRewardsPerSecond, we are not getting any gas savings out of using a local storage variable for _config, code would be shorter, and more safely changed without the local variable.
In collect rewards, the RewardCollected event is not necessarily called every time lastCollect is changed. This makes for a possible state changing function call that does not event, and prevents perfect tracking of what the future rewards will be based purely on events. That said this is only an issue when reward rate is set for some time to zero and then again raised.

Overflow

No for loops

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
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 (Deploy will be a separate PR.)

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

_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
- No for loops
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

Ethereum

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

DanielVF

Approving FixedRateRewardsSource

PraneshASP · 2024-05-09T14:25:51Z

contracts/ExponentialStaking.sol

+        uint256 oldPoints = 0;
+        uint256 oldEnd = 0;


gas: no need for initialising with default values.

PraneshASP · 2024-05-09T14:26:11Z

contracts/ExponentialStaking.sol

+        require(to != address(0), "Staking: To the zero address");
+        require(duration >= minStakeDuration, "Staking: Too short");


Consider custom errors?

PraneshASP · 2024-05-10T15:35:23Z

tests/staking/Migrator.t.sol

+            assertEq(amount, 0, "Lockup still exists");
+            assertEq(end, 0, "Lockup still exists");
+            assertEq(points, 0, "Lockup still exists");


good to add a different string as it will be easier to figure out which one fails, if one if them fail.

assertEq(amount, 0, "amount:Lockup still exists"); assertEq(end, 0, "end:Lockup still exists"); assertEq(points, 0, "points:Lockup still exists");

PraneshASP · 2024-05-10T15:36:11Z

contracts/Migrator.sol

+import "OpenZeppelin/openzeppelin-contracts@4.6.0/contracts/token/ERC20/extensions/ERC20Burnable.sol";
+import "./Governable.sol";
+
+interface IStaking {


can we move this interface to a separate (interfaces) folder?

PraneshASP · 2024-05-10T15:45:26Z

tests/staking/Migrator.t.sol

+import "contracts/tests/MockOGV.sol";
+import "contracts/tests/MockOGVStaking.sol";
+
+contract MigratorTest is Test {


good to add tests for event data as well.

PraneshASP · 2024-05-10T15:47:15Z

tests/staking/Migrator.t.sol

+        ogn.mint(address(migrator), 10000000 ether);
+
+        // Users have enough OGV
+        ogv.mint(alice, 10000000 ether);


10000000 ether seems redundant across the tests. maybe make it a constant?

PraneshASP · 2024-05-10T15:49:20Z

tests/staking/Migrator.t.sol

+        lockupIds[0] = 0;
+        lockupIds[1] = 1;
+
+        uint256 stakeAmount = (11000 ether * 0.09137 ether) / 1 ether;


0.09137 ether can be a constant in tests or can fetched from migrator.CONVERSION_RATE().

I guess one advantages of having this number in the tests as well is that it confirms that the contract also has the right conversion rate set. Somewhat like double entry accounting.

Yeah, agree with both, I can probably move it to the top of the test file as constant (still separate from the hardcoded contract value)

Add OGNRewardsSource contract

4f205b0