Implement Curve Pool Booster.

[clement-ux]

Curve Pool Booster

This contract aims to take all funds available in the contract and create/manage bribes on VotemarketV2.
As VotemarketV2 is only available in L2s, this contract connects with VM using the CampaignRemoteManager.

The connection with VM on L2s uses CCIP, so we need to add a bit of ETH in every transaction we do. The remaining gas is sent back to the contract. That's why I suggest ETH remain in the contract instead of sending transactions from the relayer with ethers.

The relayer will be able to perform multiple actions:

• Create a bribe
• Increase the amount for the remaining time
• Increase bribe duration
• Adjust bribe price
• Set bribe ID (as it uses cross-chain communication, a bribe is not created instantly, so we need to check logs and add it manually afterward).

Tests are pretty basic and don't test the whole process due to the complexity of testing cross-chain messaging. However, as this contract will hold a small amount of funds, I think it is ok to test it in prod.

Code Change Checklist

To be completed before internal review begins:

• The contract code is complete
• Executable deployment file
• Fork tests that test after the deployment file runs
• Unit tests *if needed
• The owner has done a full checklist review of the code + tests

Internal review:

• Two approvals by internal reviewers

[github-actions]

	Warnings
⚠️	👀 This PR needs at least 2 reviewers

Generated by 🚫 dangerJS against 7a85a76

[codecov]

Codecov Report

Attention: Patch coverage is 28.28283% with 71 lines in your changes missing coverage. Please review.

Project coverage is 51.93%. Comparing base (097f3f3) to head (7a85a76).

Files with missing lines	Patch %	Lines
...ontracts/contracts/strategies/CurvePoolBooster.sol	28.28%	71 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2327      +/-   ##
==========================================
- Coverage   51.94%   51.93%   -0.01%     
==========================================
  Files          91       92       +1     
  Lines        4414     4513      +99     
  Branches     1162     1195      +33     
==========================================
+ Hits         2293     2344      +51     
- Misses       2118     2166      +48     
  Partials        3        3              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sparrowDom]

Minor event comments. I guess prod testing is the way to go here like you suggest.

[shahthepro]

Why is this necessary? to recover any accidentally sent tokens? If yes, can you change it to onlyGovernor? Also, we typically have a transferTokens method for ERC20 tokens as well

[clement-ux]

to recover any accidentally sent tokens?

Not really. This contract will hold a bit of ETH, as we need to pay a bit when calling the CampaignRemoteManager (CCIP fees). So it is in the situation where the pool booster will not be used anymore (end of services) and the operator what to claim the remaining ETH.

And using a payable function is not the best (imo) because a surplus of ETH is sent back to this contract, not to the sender.

What do you think of keeping onlyOperator modifier then?

[sparrowDom]

IMO it is fine, as there will only be small amount of ETH on the contract whose purpose is to fuel the gas for transactions and that is not part of the user funds.

[clement-ux]

Yes, exactly, only to fuel gas transactions, nothing more.

[notion-workspace]

Code

[sparrowDom]

there are tokens (like ours) which don't have a perfect up to a WEI exact accounting when doing transfers and they might be doing some rounding. It could happen that when you transfer a feeAmount away, that the actual balance remaining won't be balance - feeAmount but rather balance - feeAmount - 1 WEI

For that reason I would rather return IERC20(rewardToken).balanceOf(address(this)); here

[sparrowDom]

Otherwise the whole transaction amount that was about to be transferred over to the other chain might fail

[DanielVF]

Doing the IERC20(rewardToken).balanceOf(address(this)) is good.

[clement-ux]

Done in this commit: 82770e6

[sparrowDom]

I think it would be better to replace the return balance - feeAmount with the IERC20(rewardToken).balanceOf(address(this));. Since if feeAmount == 0 then we query the rewardsToken contract for balance twice wasting some gas

[sparrowDom]

Requirements

PR is integrating with CampaignRemoteManager mainnet contract which communicates with the Votemarket contract that is located on L2. It is used to create and manage the incentives campaign.

Easy Checks

Authentication

• Never use tx.origin
• Every external/public function is supposed to be externally accessible
• Every external/public function has the correct authentication

Ethereum

• Contract does not send or receive Ethereum.
  • it does as it needs to pay the bridge fees
• Contract has no payable methods.
  • it does as it needs to be able to receive ETH
• Contract is not vulnerable to being sent self destruct ETH

Cryptographic code

• This contract code does not roll it's own crypto.
• No signature checks without reverting on a 0x00 result.
• No signed data could be used in a replay attack, on our contract or others.

Gas problems

• no for loops

Black magic

• Does not contain selfdestruct
• Does not use delegatecall outside of proxying. If an implementation contract were to call delegatecall under attacker control, it could call selfdestruct the implementation contract, leading to calls through the proxy silently succeeding, even though they were failing.
• Address.isContract should be treated as if could return anything at any time, because that's reality.

Overflow

• Code is solidity version >= 0.8.0
• All for loops use uint256
  • no for loops

Proxy

• No storage variable initialized at definition when contract used as a proxy implementation.

Events

• All state changing functions emit events

Medium Checks

Rounding and casts

• Contract rounds in the protocols favor
• Contract does not have bugs from loosing rounding precision
• Code correctly multiplies before division
• Contract does not have bugs from zero or near zero amounts
• Safecast is aways used when casting - no casting

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.

External calls

• Contract addresses passed in are validated
  • They will be on deployment review
• No unsafe external calls
  • SafeApprove requires resetting the allowance to 0 before setting a non 0 allowance
• Reentrancy guards on all state changing functions
  • most functions that interact with external contract are permissioned so they should be safe from re-entrency. Still it doesn't hurt to add re-entrency on it.
  • Still doesn't protect against external contracts changing the state of the world if they are called.
• No malicious behaviors
• Low level call() must require success.
• No slippage attacks (we need to validate expected tokens received)
• Oracles, one of:
  • No oracles
  • Oracles can't be bent
  • If oracle can be bent, it won't hurt us.
• Do not call balanceOf for external contracts to determine what they will do when they use internal accounting
  • [ x when calculating the balance without fee in _handleFee() we should query the contract for the balance once again. As rounding (e.g. in 4626 type of ERC20 contracts) can not assure that balance - X will be up to 1 WEI accurate.

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.~

Deploy

• Deployer permissions are removed after deploy

Downstream

• We have monitoring on all backend protocol's governances
• We have monitoring on a pauses in all downstream systems

Thinking

Logic

Logic is simple as it is a simple interface to a campaignRemoteManager. The one downside is that there is some manual managing of campaignId setting, but there is no other way to do this one. And we will probably need to do it only once, or very rarely.

Deployment Considerations

We should be monitoring all the bridge transactions that happen as a result of transactions.

Internal State

• Stored state represents configurable properties.
• After campaign is created the campaignId must be set. And after that all the manage campaign functions can be called.

Attack

Biggest risk here is bridge transactions failing as we will need to be able to detect that off-chain.

Flavor

Code is nice and simple.