Enable fee distribution to OGV stakers

[zack-eth]

Please see https://github.com/OriginProtocol/origin-dollar/pull/1058 (this feature requires changes in both repositories)

[franckc]

Thanks for making this change! Can we add a unit test to make sure this works as expected?

[DanielVF]

@shahthepro I think the "calcRewards()" internal method name may be a little confusing here, perhaps it should be changed to "calculateInflation()" and then the rest of code can get much clearer.

The actual rewards payout is then two independent if's. If we have balance, we pay that. If we have inflation, then we we mint the inflation. We don't need to do two balance checks, once to add in _calcRewards and once to subtract in collectRewards.

[shahthepro]

I think the "calcRewards()" internal method name may be a little confusing here, perhaps it should be changed to "calculateInflation()" and then the rest of code can get much clearer.

The actual rewards payout is then two independent if's. If we have balance, we pay that. If we have inflation, then we we mint the inflation. We don't need to do two balance checks, once to add in _calcRewards and once to subtract in collectRewards.

@DanielVF Have changed the method name _calculateInflation() and have also removed the balance check from this method. However, I had to add the balance check to previewRewards() method, which seems to be used by other contracts (OgvStaking.sol) and the DApp

[DanielVF]

We'll need to make sure slither checks pass before merging to master. In this case is concerned that we are ignoring the final amount from the swap. We don't actually need it, since the check on min size is handled by the swap, so we can safely ignore it.

I think the right comment is something like // slither-disable-next-line slither-disable-next-line

[shahthepro]

I think the right comment is something like // slither-disable-next-line slither-disable-next-line

Yep, // slither-disable-next-line unused-return works

[DanielVF]

We'll need a deploy test for this PR.

Would also be good to paste the fork tests into the issue so others can play with them.

[shahthepro]

Have added the deploy script. Copying my own message from Discord:

• Tried using the map.json file under build/deployments but it didn't have addresses of all deployed contracts. So have just hardcoded the addresses of OGV and RewardsSourceProxy contracts in the code
• Looks like a Multi-sig wallet is the governor of RewardsSourceProxy on mainnet, so there's no governance proposal for this one (let me know if I missed something here)

Also, here's the script I used to test the deployment on fork

import os
from brownie import *

def main():
    deployer = accounts.add(os.environ["DEPLOYER_KEY"])
    FROM = {'from': deployer}
    OGV_PROXY_ADDRESS = "0x9c354503c38481a7a7a51629142963f98ecc12d0"
    rewards = run("deploy_002_upgrade_rewards_source")

    ogv = OriginDollarGovernance.at(OGV_PROXY_ADDRESS)
    governor = accounts.at(rewards.governor(), force=True)

    # Allow deployer to mint
    ogv.grantMinterRole(deployer.address,{'from':governor})

    # Fetch current balance and rewards
    (curr_balance, curr_rewards) = fetch_balance(ogv, rewards)

    # Send some OGV to the new contract
    print("Minting 10k OGV to contract")
    ogv_to_mint = 1e4 * 1e18
    ogv.mint(rewards.address, ogv_to_mint, FROM)

    # Fetch new balance
    (new_balance, new_rewards) = fetch_balance(ogv, rewards)

    # Check if any balance added to the contract 
    # is being calculated in the rewards
    print("Expected Balance:", curr_balance + ogv_to_mint)
    print("Actual Balance:  ", new_balance)

    print("Expected Rewards:", curr_rewards + ogv_to_mint)
    print("Actual Rewards:  ", new_rewards)

def fetch_balance(ogv, rewards):
    curr_balance = ogv.balanceOf(rewards.address)
    curr_rewards = rewards.previewRewards()

    print("Current Balance:", curr_balance)
    print("Current Rewards:", curr_rewards)

    return (curr_balance, curr_rewards)

[DanielVF]

I'll have to give this a full review another day, but it's looking good so far. Thanks!

[DanielVF]

Requirements

This PR handles inserting OGV bought back back from users into the existing rewards distribution process. Any OGV sent to the rewards contract is passed on to the staking contract as addtional rewards on top of inflation.

Invariants

• [x] The amount of OGV given to the rewards contract (from inflation+other) should be equal to number returned by collectRewards().
• [x] The amount returned from previewRewards() should match the amount returned from an immediate collectRewards();
  • Lol, as long as we don't set the inflation contract as its own target contract to send funds too.
• [x] The OGV balance of the contract should always be zero after collect rewards

Internal State

See Invariants section above

• [x] Should be fine operating with or without an OGV balance

Skiping reviewing other contract state, which is not changed by this PR.

Attack

If there was some way to increase the amount minted, that would be bad. But this code makes no changes to those calculations.

Logic

Good

Tests

• [x] Each publicly callable method has a test
• [x] Each logical branch has a test
• [x] Edge conditions are tested

Deployment Considerations

None

Flavor

Code is simple.

Crashes in collectRewards do not affect staking contract. Crashes in previewRewards would affect previewing in staking contract, but nothing else.

Overflow

• [x] Never use "+" or "-", always use safe math
• [x] Check that all for loops use uint256

Black magic

No magic

Dependencies

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

Deploy

Deploy process transfers ownership of the implimentation contract to current governance. This is probably unnessasay, but won't hurt.

Authentication

No changes to authentication

Cryptographic code

No crypto

Gas problems

No for loops

External calls

Only addtional external calls are to balanceOf() and transfer() on OGV.

Ethereum

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

[DanielVF]

Correct, solidity >= 0.8 means no safe math needed.