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.

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

Update initializer modifier to prevent reentrancy during initialization #57

Closed
code423n4 opened this issue Feb 16, 2022 · 4 comments
Closed
Labels
bug Something isn't working invalid This doesn't seem right

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/aave/lens-protocol/blob/main/package.json#L74
https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/upgradeability/VersionedInitializable.sol#L20-L21
https://github.com/aave/protocol-v2/blob/6a503eb0a897124d8b9d126c915ffdf3e88343a9/contracts/protocol/libraries/aave-upgradeability/VersionedInitializable.sol#L2
https://github.com/aave/protocol-v2/commits/6a503eb0a897124d8b9d126c915ffdf3e88343a9/contracts/protocol/libraries/aave-upgradeability/VersionedInitializable.sol
OpenZeppelin/openzeppelin-contracts#3006

Vulnerability details

This is mainly a risk I feel should be brought to the sponsor's and the judge's attention. I can't fully exploit and remediate to this, but I'll try to be as clear as possible in every steps, as it might be a high risk finding.

The issue

The solution uses "@openzeppelin/contracts": "4.4.0" (https://github.com/aave/lens-protocol/blob/main/package.json#L74)

This version has a known high risk vulnerability on initializer: https://snyk.io/test/npm/@openzeppelin/contracts/4.4.0 . This finding was published and disclosed around December 15th 2021. Here's the pull request: OpenZeppelin/openzeppelin-contracts#3006

However, this solution doesn't use OpenZeppelin's Initializable: it instead implements its own VersionedInitializable, inspired from Aave's version: https://github.com/aave/protocol-v2/blob/6a503eb0a897124d8b9d126c915ffdf3e88343a9/contracts/protocol/libraries/aave-upgradeability/VersionedInitializable.sol#L2

But this Aave Initializable is still inspired from the old OpenZeppelin's Initializable, and the last commit is from January 27th 2021 (almost a year before the 4.4.0's vulnerability discovery) : https://github.com/aave/protocol-v2/commits/6a503eb0a897124d8b9d126c915ffdf3e88343a9/contracts/protocol/libraries/aave-upgradeability/VersionedInitializable.sol .

With these lack of up-to-date information / versions and commit in mind: I believe the same vulnerability discovered in OpenZeppelin's Initializable 2 months ago can be replicated here and should be paid attention to.

While this is certainly a potential finding found by searching, a more experienced security researcher is needed to fully prove with a POC that VersionedInitializable is indeed vulnerable.

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Feb 16, 2022
code423n4 added a commit that referenced this issue Feb 16, 2022
@Zer0dot
Copy link
Collaborator

Zer0dot commented Mar 18, 2022

This is valid, but the scope is restricted to having a corrupt proxy admin, as during deployment initialization is done in the proxy constructor (and even if it wasn't, there is basically nothing at risk during initial deployment).

@Zer0dot Zer0dot added disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons labels Mar 18, 2022
@0xleastwood
Copy link
Collaborator

While I appreciate the in depth finding, I don't see any external calls made in any of the initialize() functions. As such, this finding doesn't have a lot of validity but I think it is fair to include this in the warden's QA report.

@0xleastwood
Copy link
Collaborator

As far as I'm aware, contracts containing an initialize() function will be part of a proxy construction which doesn't suffer from the issue raised by the warden.

@0xleastwood 0xleastwood added QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 3 (High Risk) Assets can be stolen/lost/compromised directly disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) labels May 4, 2022
@0xleastwood
Copy link
Collaborator

0xleastwood commented May 5, 2022

Merging into #60 and marking as invalid.

@0xleastwood 0xleastwood added invalid This doesn't seem right and removed QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons labels May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

3 participants