Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

AaveV3LeverageStrategyExtension audit feedback #142

Merged
merged 29 commits into from
Aug 14, 2023
Merged

AaveV3LeverageStrategyExtension audit feedback #142

merged 29 commits into from
Aug 14, 2023

Conversation

ckoopmann
Copy link
Collaborator

@ckoopmann ckoopmann commented Jun 15, 2023 •
edited by pblivin0x
Loading

@ckoopmann ckoopmann marked this pull request as draft June 15, 2023 08:42
@coveralls
Copy link

coveralls commented Jun 15, 2023 •
edited
Loading

Coverage Status

coverage: 76.259% (-0.9%) from 77.139% when pulling 0bc8b24 on aave-v3-leverage-strategy-extension-audit-feedback into 8727853 on master.

This was referenced Jul 4, 2023
Open
Open
Open
This was referenced Jul 4, 2023
Closed
Closed
@ckoopmann ckoopmann marked this pull request as ready for review July 7, 2023 03:04
FlattestWhite
FlattestWhite reviewed Jul 31, 2023
View reviewed changes
contracts/adapters/AaveV3LeverageStrategyExtension.sol Outdated

constructor(
IBaseManager _manager,
IAaveOracle _aaveOracle,
Copy link
Contributor

Choose a reason for hiding this comment

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

The aaveOracle should come from calling getPriceOracle on the LendingPoolAddressProvider

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, thanks. Will adjust 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adjusted in:
5d1cc59

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In fact I went one step further and just read the address anew from the addressProvider everytime when I call the oracle.
Thereby we don't get rugged if for some reason the AaveOracle address changes:

0a69def

@pblivin0x pblivin0x self-requested a review August 1, 2023 20:35
pblivin0x
pblivin0x requested changes Aug 1, 2023
View reviewed changes
Copy link
Contributor

@pblivin0x pblivin0x left a comment

Choose a reason for hiding this comment

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

All changes here LGTM 💯

Before final approval and merge, lets make sure all dependencies in our integration tests are up to date with the index-protocol audit fixes IndexCoop/index-protocol@520a9cf

  • AaveV3LeverageModule
  • DebtIssuanceModuleV2
  • Invoke

@ckoopmann
Copy link
Collaborator Author

ckoopmann commented Aug 2, 2023 •
edited
Loading

All changes here LGTM 💯

Before final approval and merge, lets make sure all dependencies in our integration tests are up to date with the index-protocol audit fixes IndexCoop/index-protocol@520a9cf

  • AaveV3LeverageModule
  • DebtIssuanceModuleV2
  • Invoke

In the integration tests for the StrategyExtension I am testing against deployed contracts. (DebtIssuanceModule, SetTokenCreator etc.)
Therefore to make sure those are testing against the correct versions with the fixed invoke issue, we need to redeploy those first.

Basically we will have to update the addresses here and also adjust the test such that it just uses the new A3LM deployment instead of redeploying the module.

@pblivin0x
Copy link
Contributor

All changes here LGTM 💯
Before final approval and merge, lets make sure all dependencies in our integration tests are up to date with the index-protocol audit fixes IndexCoop/index-protocol@520a9cf

  • AaveV3LeverageModule
  • DebtIssuanceModuleV2
  • Invoke

In the integration tests for the StrategyExtension I am testing against deployed contracts. (DebtIssuanceModule, SetTokenCreator etc.) Therefore to make sure those are testing against the correct versions with the fixed invoke issue, we need to redeploy those first.

Basically we will have to update the addresses here and also adjust the test such that it just uses the new A3LM deployment instead of redeploying the module.

Changes implemented here #145

@pblivin0x
fix(dependencies): Add post audit Aave V3 dependencies (#145)
998fd21 
update post audit contracts for integration tests

Co-authored-by: Pranav Bhardwaj <[email protected]>
@ckoopmann
Copy link
Collaborator Author

All changes here LGTM 💯
Before final approval and merge, lets make sure all dependencies in our integration tests are up to date with the index-protocol audit fixes IndexCoop/index-protocol@520a9cf

  • AaveV3LeverageModule
  • DebtIssuanceModuleV2
  • Invoke

In the integration tests for the StrategyExtension I am testing against deployed contracts. (DebtIssuanceModule, SetTokenCreator etc.) Therefore to make sure those are testing against the correct versions with the fixed invoke issue, we need to redeploy those first.
Basically we will have to update the addresses here and also adjust the test such that it just uses the new A3LM deployment instead of redeploying the module.

Changes implemented here #145

Noted that in the linked PR only the A3LM json-file / bytecode is updated. I guess technically we would also want to update the DIM and other modules that use the invoke library. But this is nothing that should necessarily block us from merging this I guess.

@pblivin0x pblivin0x self-requested a review August 14, 2023 13:13
pblivin0x
pblivin0x approved these changes Aug 14, 2023
View reviewed changes
Copy link
Contributor

@pblivin0x pblivin0x left a comment

Choose a reason for hiding this comment

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

LGTM 💯

Noting the open pr for test additions in #146

snake-poison and others added 3 commits August 14, 2023 21:16
@snake-poison @ckoopmann
Updates to test (#146)
8ef6b7b 
* remove test filter on FlashMintLeveraged.

* chore: update current block

* test: updates to deployment of leverage module.

* Fix failing tests for AaveV3LeverageStrategyExtension (#147)

* Fix failing tests

* Adjust test message for ripcord test

---------

Co-authored-by: christn <[email protected]>
@snake-poison
remove .only from test
125323c
@snake-poison
test: fix issues with updating block.
0bc8b24 
Made deadline on uniswap router swaps less brittle.
snake-poison
snake-poison approved these changes Aug 14, 2023
View reviewed changes
Copy link
Contributor

@snake-poison snake-poison left a comment

Choose a reason for hiding this comment

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

LGTM!

@snake-poison snake-poison merged commit 83e500f into master Aug 14, 2023
2 checks passed
@snake-poison snake-poison deleted the aave-v3-leverage-strategy-extension-audit-feedback branch August 14, 2023 17:15
@FlattestWhite
Copy link
Contributor

🎉 This PR is included in version 0.15.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FlattestWhite FlattestWhite FlattestWhite approved these changes

@snake-poison snake-poison snake-poison approved these changes

@pblivin0x pblivin0x pblivin0x approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ckoopmann @coveralls @pblivin0x @FlattestWhite @snake-poison