Badger-Finance / badger-strategies

5 stars 1 forks source link

[ETH] byvWBTC GAC and treasury withdrawal fee upgrade #65

Closed btcookies closed 2 years ago

btcookies commented 2 years ago

Strategy Review

Description

Upgrading the SimpleWrapperGatedUpgradeable contract for byvWBTC to support GAC pausing and ability to redirect withdrawal fees. To redirect withdrawal fees, new storage variable treasury was created with new setter and event.

Issue described in detail here https://github.com/Badger-Finance/badger-multisig/issues/540.

Code Link

Commit / Hash / Repo should be known from this.

Due Diligence Document (Link, but could also just be a .md file in repo)

This document evaluates the protocols that are interacted with as strategy positions.

Deployed Contract (If present)

https://etherscan.io/address/0x4b92d19c11435614CD49Af1b589001b7c08cD4D5

Review By

Ongoing:
Completed:

Review by Security Board

Not started / ongoing / completed

Test Checks (screen shot of all test passing)

btcookies commented 2 years ago

cc: @sajanrajdev @shuklaayush @GalloDaSballo @axejintao

GalloDaSballo commented 2 years ago

@btcookies is there a upgrade test?

btcookies commented 2 years ago

@GalloDaSballo I thought I baked it into each of the tests by upgrading, comparing storage vars, testing deposit withdraw etc

shuklaayush commented 2 years ago

LGTM

Adds a treasury address storage var that withdrawal fees are transferred to
Adds GAC pausing and blacklisting to functions

GalloDaSballo commented 2 years ago

For some reason governance is called affiliate beside that LGTM

sajanrajdev commented 2 years ago

For some reason governance is called affiliate beside that LGTM

This is the name originally introduced by Yearn, the wrapper is based on their AffiliateToken contract.

sajanrajdev commented 2 years ago

Review

Adds a storage variable for the treasury with a setter only callable by Affiliate. The withdrawal function was modified to direct withdrawal fees to this address (And ensures that it is not zero).
Adds GAC pausability and blacklisting

Notes

The treasury must be set to Treasury Ops atomically with the upgrade. For as long as the treasury variable is set to 0x0 no withdrawal fees will be charged.

Testing

Tests are included to confirm a successful upgrade as well of the introduced features and all of the regular operations. All tests are passing:

LGTM

GalloDaSballo commented 2 years ago

Closing as succesful