[Review]: Voter Module

[petrovska-petro]

Module Review

Description

README

Code Link

release for review

Review By

Requesting green light from solidity pod for the code above and discussed during solidity office hours

• Ongoing:
• Completed:

Review by Security Board

• Not started / ongoing / completed

Test Checks

[okjintao]

Findings

Review will be broken down into the labeled sections of the contract

Constants Variables

• correct address for aura token
• correct address for aurabal token
• correct address for aura locker
• correct address for gravi + strategy

State Variables

• interval variable could have a more descriptive name

Events

• RewardPaidModule would like to see this as RewardPaid just to not make it confusing w/ the module part
• _reward concept is covered in the name, with a clearer event name we could use _amount
• GuardianUpdated only event w.o. timestamp, should this be added for consistency?

Keepers - Executors

• invariant function duplicated across checks, can be made a view call

rewards > 0 && (block.timestamp - lastRewardClaimTimestamp) > interval

• _withdrawGraviAndLockAura may have a bug with no impact, just might not be intended

Invariant Check on Balance

Assume

• graviSafeBal = 0.5 (5e17)
• totalWdAura = 1 (1e18)
• graviPpfs = 1.234

(graviSafeBal / ONE_ETH) * graviPpfs

you'd end up with 0 * graviPpfs and so for any value < 1 ether

totalWdAura < Check will never be true

This only really applied is there was say, 0.25 WD Aura, and then you end up encoding the request to withdraw the proper amount as the multiplication math is good and you'd break.

I believe the correct way would just be to do

(graviSafeBal * graviPpfs) / ONE_ETH

[shuklaayush]

• I think pending rewards should be calculated by querying the claimableRewards() instead of userData() (as in balanceOfRewards of the graviAura strat). userData would give a stale value
• As jintao alluded, I think it's better to do exact calculations by doing graviSafeBal * GRAVI.balance() / GRAVI.totalSupply() instead of using ppfs
• Can replace abi.encodeWithSelector with abi.encodeCall which imo is easier to read

[GalloDaSballo]

Will review post the above edits

[petrovska-petro]

thanks guys for the review. created another release adding the suggestions in the above comments:

latest release for review

cc: @shuklaayush @axejintao @GalloDaSballo

[GalloDaSballo]

Fixes LGTM, added specific test coverage here: https://github.com/petrovska-petro/VoterModule/pull/1

I think it's good to go for a road test

[shuklaayush]

• Change checkUpkeep condition to graviBal > 0 && totalWdAura > 0
• Since anyone can claim rewards for anyone else, maybe the condition should be earnData[0].amount > 0 || auraBal.balanceOf(address(SAFE)) > 0

[petrovska-petro]

• Change checkUpkeep condition to graviBal > 0 && totalWdAura > 0
• Since anyone can claim rewards for anyone else, maybe the condition should be earnData[0].amount > 0 || auraBal.balanceOf(address(SAFE)) > 0

added your notes in latest release

[petrovska-petro]

final release addressing extra notes from @shuklaayush

[shuklaayush]

LGTM

[petrovska-petro]

thanks for the reviews, deployed the contract at: https://etherscan.io/address/0xc485afd2f3252ccb69d1c94392701d51013d42eb#contracts

will commence with the integration over the multisig repo issue

[GalloDaSballo]

@petrovska-petro I've forwarded to Watchpug, will ping if anything is off