Skip to content

Conversation

@sparrowDom
Copy link
Member

@sparrowDom sparrowDom commented Dec 20, 2022

LUSD deployment artifacts from this pr: #1205

Contracts

Name Address
ConvexLUSDMetaStrategyProxy 0x7a192dd9cc4ea9bdedec9992df74f1da55e60a19
ConvexGeneralizedMetaStrategy 0xB6764c2Cc8F1fDCd89Af1C3e246f886579746233

Copy link
Collaborator

@shahthepro shahthepro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a quick pass. The contracts are verified on Etherscan and the source matches codebase. Also, the order of deployment transactions seem to be right

slither:
name: "Slither"
runs-on: ubuntu-latest
# As long as we need Python 3.6 here in the test, we can only use up to Ubuntu 20.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating this. We've a low-priority issue to replace Slither with dependabot in future, until then this should work

@DanielVF
Copy link
Contributor

DanielVF commented Dec 27, 2022

  • Deployed contract verified code (and dependancies) match the final PR commit's code
  • 🛑The transactions that interacted with the newly deployed contract match the deploy script.
  • Governance proposal matches the deploy script: Proposal 41 TX
  • Smoke tests pass after fork test execution of the governance proposal
  • Update Docs

Deployment matches

function compare_contracts; git diff --color-words --no-index ./$argv PATH_TO_CONTRACTS/$argv; end
function compare_ozcontracts; git diff --color-words --no-index ./$argv PATH_TO_CONTRACTS/node_modules/$argv; end

cast etherscan-source -d proxy 0x7A192DD9Cc4Ea9bdEdeC9992df74F1DA55e60a19
cd proxy/ConvexLUSDMetaStrategyProxy
find . | grep .sol
for i in (find . | grep .sol | grep -v openzeppelin); compare_contracts $i; end
for i in (find . | grep .sol | grep openzeppelin); compare_ozcontracts $i; end

cast etherscan-source -d impl 0xB6764c2Cc8F1fDCd89Af1C3e246f886579746233
cd impl/ConvexGeneralizedMetaStrategy/
find . | grep .sol
for i in (find . | grep .sol | grep -v openzeppelin); compare_contracts $i; end
for i in (find . | grep .sol | grep openzeppelin); compare_ozcontracts $i; end

Transactions Check

🛑 The deployer transaction to transfer governance, transfers ownership of the strategy proxy to deployer address rather than to the governor contract. https://etherscan.io/tx/0xf99cc2433a0d2984e67c44dfc1c3d8d6749f8b2bd17ec9b8f9d200ba143b8999

This also causes the governance proposal to fail, given that the governor cannot accept ownership.

image

I'm guessing this is because both the deployer address and the governor address config were changed for the deploy, when only the deployer address needed to be changed.

We can fix by having this deployer address send the correct change governance transaction.

Governance Proposal Check

from world import *
v = governor.getActions(41)
[print(v[0][i],v[1][i],v[2][i],) for i in range(0, len(v)+1)]

Smoke tests

from world import *

DEPLOYER = '0xFD9E6005187F448957a0972a7d0C0A6dA2911236'
LUSD_STRAT = '0x7a192dd9cc4ea9bdedec9992df74f1da55e60a19'
 
lusd_strat = Contract.from_explorer(LUSD_STRAT)
lusd_strat.transferGovernance(GOVERNOR, {'from': DEPLOYER})  #TODO: Needs to be live
sim_governor_execute(41)

# -----

before = vault_core.totalValue()
vault_admin.reallocate(MORPHO_COMP_STRAT, LUSD_STRAT, [USDC], [1e6 * 1e6], {'from': STRATEGIST})
after = vault_core.totalValue()
print("Profit In", c18(after-before))

vault_admin.reallocate(LUSD_STRAT, MORPHO_COMP_STRAT, [USDC], [lusd_strat.checkBalance(USDC) * 3], {'from': STRATEGIST})
after2 = vault_core.totalValue()
print("Profit Almost Full Cycle", c18(after2-before))

vault_admin.withdrawAllFromStrategy(LUSD_STRAT, {'from': STRATEGIST})
after3 = vault_core.totalValue()
print("Profit Full Cycle", c18(after3-before))

@franckc
Copy link

franckc commented Dec 27, 2022

Great catch @DanielVF ! That could have been a serious security vulnerability.

@sparrowDom How abou

  • we clean up the playbook to be clear about what needs to be set?
  • also, is there any safety check we should add to the deploy script so that it issues a warning or does not run at all if it detects the network is mainnet and the deployer address is also being used as governor address?

Some additional context on the discord discussion: https://discord.com/channels/404673842007506945/965649970269143120/1057400565719638046

Also CC @shahthepro so that we don't make the same mistake when deploying the upcoming Morpho Aave strategy

@DanielVF
Copy link
Contributor

Perhaps we could have the deployer address set by the deployer private key when it is set. This way there's no config file to change during deployments, and there's not really anything to mess up.

@DanielVF DanielVF changed the title Sparrow dom/deploy 45 - LUSD Deploy 45 - LUSD Dec 29, 2022
@sparrowDom sparrowDom mentioned this pull request Jan 3, 2023
@sparrowDom
Copy link
Member Author

Proposed solution that adds a safeguard: #1211

@sparrowDom sparrowDom force-pushed the sparrowDom/deploy_45 branch from 2f26477 to bd6a5df Compare January 3, 2023 16:15
@sparrowDom
Copy link
Member Author

This transaction correct the wrong pending governor on the LUSD strategy contract

@DanielVF DanielVF merged commit e550ea2 into master Jan 3, 2023
@DanielVF DanielVF deleted the sparrowDom/deploy_45 branch January 3, 2023 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants