Val's peer review (LlamaRisk)

[0xValJohn]

Reviewed commit hash 35140e5

SingleCampaign.vy / Distributor.vy

1. Question: Would there ever be a need to add/remove/change the guard?
2. Suggestion: Rename the `execute()`` public function to something more descriptive
3. Question: Why is distribute_reward() external, shouldn't it be `@internal?
4. Question: Should recover_token be allowed to sweep the reward_token?
5. Question: Is a reentrancy guard needed between the 2 contracts?
6. Suggestion: Add reward validation. There is no validation that the Distributor contract has sufficient reward tokens to cover the total rewards before setting epochs.
7. Suggestion: Add a get_rewards_summary(), @return (total_rewards, remaining_rewards, remaining_epochs)

Deployment script

1. Suggestion: Add Deployment Pre-Checks

def check_contract_dependencies(deployment_type):
    if deployment_type == "distributor":
        if not os.getenv("SINGLE_CAMPAIGN_ADDRS"):
            raise EnvironmentError("Must deploy SingleCampaigns first")

2. Suggestion: Add Dry-Run Mode

@click.option("--dry-run", is_flag=True, help="Simulate deployment without broadcasting")
def deploy(dry_run):
    if dry_run:
        return account.broadcastless_deploy(...)

Tests

• Please post the output of running the full test suite.

[martinkrung]

Reviewed commit hash 35140e5

SingleCampaign.vy / Distributor.vy

1. Question: Would there ever be a need to add/remove/change the guard?

1. I did avoid that as a design choice to reduce attack vectors.
2. These contracts are most only active for 1-4 months and with no token owned later. (If not all token used, they are sent back to the recovery address)

2. Suggestion: Rename the `execute()`` public function to something more descriptive

The real function is to call distribute_reward()

execute() is a pattern used to have others discover that and execute. some kind of keeper pattern. my research showed that execute() is the standard naming convention.

The idea is to donnate crvUSD to the campaign contract, on execute everyone can earn that crvUSD, while on distribute_reward() you will not.

3. Question: Why is distribute_reward() external, shouldn't it be `@internal?
   see above

4. Question: Should recover_token be allowed to sweep the reward_token?

yes, this function is used if campaign is stopped and then send back the reward_token to the entity giving the. recovery address is normally the sending entity

5. Question: Is a reentrancy guard needed between the 2 contracts?

to my knowledge not, as there are no call backs? SingleCampaign calls Distributor, but nothing is called back?

6. Suggestion: Add reward validation. There is no validation that the Distributor contract has sufficient reward tokens to cover the total rewards before setting epochs.

Thats by desing. At the time of setting epochs, the distributor contract should be empty. This way every decisions on the reward is done before there is any tokens. So nobody is managing any value activly)

7. Suggestion: Add a get_rewards_summary(), @return (total_rewards, remaining_rewards, remaining_epochs)

Good idea, I may add later

Deployment script

1. Suggestion: Add Deployment Pre-Checks

def check_contract_dependencies(deployment_type):
if deployment_type == "distributor":
if not os.getenv("SINGLE_CAMPAIGN_ADDRS"):
raise EnvironmentError("Must deploy SingleCampaigns first")

agree, now setup has some complexity and is usable for me, but other may struggle. this woluld help.

2. Suggestion: Add Dry-Run Mode

@click.option("--dry-run", is_flag=True, help="Simulate deployment without broadcasting")
def deploy(dry_run):
if dry_run:
return account.broadcastless_deploy(...)

I have a dry mode in a different script (not commited), but this is a good hint how to make this better

Tests

• Please post the output of running the full test suite.