[bveCVX] BribesProcessor Integration | Bribes Processor Code Review

[GalloDaSballo]

Strategy Review

BribesProcessor Code Review

Description

Bribes Processor, allows Multi to sell bribes but limits their rugging ability

Code Link

Code https://github.com/GalloDaSballo/fair-selling/tree/rc.4 Integration https://github.com/GalloDaSballo/vested-cvx/tree/cvx.1.7

Deployments

Newer V0.1 Deployments onChainPricerLenient: https://etherscan.io/address/0xbab7f98d62479309219b2cc5026d4ad1c6c05674

New VotiumBribesProcesso: https://etherscan.io/address/0xb2Bf1d48F2C2132913278672e6924efda3385de2

bveCVX v1.7.1 Logic: https://etherscan.io/address/0x86ca553D5Ae7cD0005552D6E275786D5043800Bd

OLD Deployment: onChainPricer OLD: https://etherscan.io/address/0xea7cf04df34812ec26492b6b6aa90825645b641c

VotiumBribesProcessor: https://etherscan.io/address/0xbed8f323456578981952e33bbfbe80d23289246b#code

bveCVX v1.7: https://etherscan.io/address/0x57961a757ba249e616c1940548401b7cdf83a849

[GalloDaSballo]

@sajanrajdev @shuklaayush @axejintao @petrovska-petro @jorijnsmit thoughts before integrating?

[GalloDaSballo]

From @axejintao

• cow swap seller look good
• on chain pricer makes sense, i guess exclude univ3 bc of fee pools complexity

processor

• L80 has old reference to days amt
• can add a sendToGovernance natspec param, i see what it does but don't really understand is that not a rug? dev multi is trusted to do the right thing then?
• from a code perspective you could leave fn scoped toEmit and ops_fee outisde the if / else scope and then remove the duplicated emits

[sajanrajdev]

Bribes Processor review:

VotiumBribesProcessor.sol

• All hardcoded addresses LGTM
• The hardcoded STRATEGY points to the latests vested.cvx Strat which, if migrated once more, this contract would become obsolete. For security reasons it may still be good to just deploy a new Processor contract if that's the case and make the processor variable changeable at the Strategy level.
• It should also be noted that the Ops fee should be 5% of the total bribes amount in the form of bveCVX. In this case, the fee is taken from the total CVX amount obtained which is not the total Bribes amount. Therefore, charging the hardcoded 5% on CVX won't equate to the expected 5% of the total. Maybe I am missing something? From the BIP:

CowSwapSeller.sol

• All hardcoded addresses LGTM

LGTM

OnChainPricingMainnet.sol

• All hardcoded addresses LGTM

LGTM

Testing:

• test_emit.py: LGTM
• test_ragequit.py: LGTM
• test_sell_bribes_weth.py: LGTM
• test_swap_weth_badger_cvx.py: LGTM
• test_cowswap_lifecycle.py: LGTM

A few unit tests are missing as pointed out on the test_reverts.py suite, although core functionalities and integrations are covered.

Test results: The following tests are failing:

The first two seem to fail with an issue with the API, the third reverts at: require(checkCowswapOrder(orderData, orderUid));. Will investigate further...

[GalloDaSballo]

• It should also be noted that the Ops fee should be 5% of the total bribes amount in the form of bveCVX. In this case, the fee is taken from the total CVX amount obtained which is not the total Bribes amount. Therefore, charging the hardcoded 5% on CVX won't equate to the expected 5% of the total. Maybe I am missing something? From the BIP:

Please refer to the math that @jorijnsmit came up with: https://github.com/Badger-Finance/badger-multisig/blob/eb27eb035878264d41ab368e1d63306da32a61da/scripts/badger/swap_bribes_for_bvecvx.py#L104

Ultimately if the split was 75/25 then that math helps figure out the 5% fee for total bribes, taken exclusively in CVX

[GalloDaSballo]

@sajanrajdev @axejintao @shuklaayush @jorijnsmit See updated Release that addresses the issues you've raised (optimization, docs and issue with approvals) https://github.com/GalloDaSballo/fair-selling/tree/rc.1

[GalloDaSballo]

Updated to rc.2 with fix to access control, received informal signoff from team

[GalloDaSballo]

Deployment: onChainPricer: https://etherscan.io/address/0xea7cf04df34812ec26492b6b6aa90825645b641c

VotiumBribesProcessor: https://etherscan.io/address/0xbed8f323456578981952e33bbfbe80d23289246b#code

TODO: [X] Write integration in bveCVX [] Pass ownership of Processor to TechOps [] Upgrade bveCVX

[shuklaayush]

LGTM

• Moves the bribe processing logic on-chain using cow-swap
• Adds a rage-quit function in case bribes aren't processed within a fixed duration

Tests passing

[GalloDaSballo]

Waiting for Peer Review of Integration here: https://github.com/GalloDaSballo/vested-cvx/pull/5

@shuklaayush @dapp-whisperer @jorijnsmit @petrovska-petro

If this LG, last step is for me to renounce processor to MultiSig and then we can queue upgrade

[GalloDaSballo]

Release: https://github.com/GalloDaSballo/vested-cvx/tree/cvx.1.7

[GalloDaSballo]

NOTE: Future versions should emit by using this contract (until we emit to BadgerRewards) https://etherscan.io/address/0xa84b663837d94ec41b0f99903f37e1d69af9ed3e#code

[GalloDaSballo]

Updated: New processor with proper approvals as well as the harvest_forwarder https://etherscan.io/address/0xb2Bf1d48F2C2132913278672e6924efda3385de2#code

Latest Code: https://github.com/GalloDaSballo/fair-selling/tree/v0.1

[GalloDaSballo]

Updated bveCVX: https://github.com/GalloDaSballo/vested-cvx/pull/6

Waiting for review by @sajanrajdev @axejintao @shuklaayush and we can queue upgrade for Thursday

[shuklaayush]

Changes to bribes processor

• Fix allowance of relayer and add ability to set custom allowance
• Forward emissions to separate Harvest Forwarder contract
• Give 5% of bribes in badger to bvecvx-cvx lp vault holders
• Add a lenient pricer that allows max 5% slippage

Changes to strategy

• Return 0 on harvest
• Update processor to above

LGTM

[sajanrajdev]

Bribes processor Changes

CowSwapSeller:

• Adds the fee amount to the selltoken's approvals

VotiumBribesProcessor:

• Adds a function to set custom allowances to the CS relayer.
• HarvestForwarder introduced so that emissions can be sent to the BadgerTree paired to a TreeDistribution event. NOTE: The processor also checks the the HARVEST_FORWARDER.badger_tree is correct before calling distribute. This is very important since the processor is ownd by an EOA which, if compromised, could change the assets' desitnation. I would still recommend that we delegate ownership of the harvest forwarder to a multi such as TechOps.
• All tree distributions now go through the forwarder (including when ragequitting)

LGTM

Vested CVX Strat Changes

• Changed BribesProcessor address to contract with new logic from above.
• Make strategy return and report 0 upon harvest.

LGTM

NOTES

• I recommend that we include the changes required for handling the cvxFXS emitted to vlCVX, as stated in this issue, before deploying the vestdCVX logic.

[GalloDaSballo]

Updated bveCVX Logic to use newer Bribes Processor: https://etherscan.io/address/0x86ca553D5Ae7cD0005552D6E275786D5043800Bd

[GalloDaSballo]

Diff: https://www.diffchecker.com/wDz9XyzR

[GalloDaSballo]

Delegation to Tech_ops: https://etherscan.io/tx/0x64364386745cca6f927f6fe73887a87bfa7ac73a127f783cab7ef1e5514bf926

[GalloDaSballo]

Closing as it's live and seems to be working well