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

Error in modifier ifAdmin (using older versions of Openzeppelin Upgradable Contracts), affected by possible collision of function signature #243

Closed
code423n4 opened this issue Sep 7, 2023 · 5 comments
Closed

Error in modifier ifAdmin (using older versions of Openzeppelin Upgradable Contracts), affected by possible collision of function signature #243

code423n4 opened this issue Sep 7, 2023 · 5 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue edited-by-warden grade-c low quality report This report is of especially low quality primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax unsatisfactory does not satisfy C4 submission criteria; not eligible for awards

Comments

@code423n4
Copy link
Contributor

code423n4 commented Sep 7, 2023 •
edited by code4rena-admin
Loading

Lines of code

https://github.com/code-423n4/2023-09-ondo/blob/main/contracts/external/openzeppelin/contracts/proxy/TransparentUpgradeableProxy.sol#L48

Vulnerability details

In the PR of TransparentUpgradeableProxy (OpenZeppelin/openzeppelin-contracts#4154)
it's indicated that the ifAdmin modifier should not be used because if the implementation contract has a same function selector it can block it (Denial of service).

Proof of Concept

In a future implementation of rUSDY the developer would mistakenly create the function upgradeTo(address) or some function name that would return the same function signature 0x3659cfe6 (such as upgradeTo_790AA3D()) would fail because it would execute the proxy function where the user does not have permissions.
Attached is the test code where the new implementation uses upgradeTo_790AA3D() and calling the function gives error.

https://gist.github.com/Thaddeus19/8d8d3c1a313eb5fedb801db0816bdcaa

https://ipfs.moralis.io:2053/ipfs/QmQ9djbE6o4w7TwRdHkYkRSCK1XjyEiqgPVD5cwpJZwKYg/BugUpgrade.png

Tools Used

Manual Code Review

Recommended Mitigation Steps

Upgrade to the latest version of Openzeppelin contracts (v4.9.0 at the time of the report).

Assessed type

Other
@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Sep 7, 2023
code423n4 added a commit that referenced this issue Sep 7, 2023
@code423n4
Team-Slayer issue #243
9d76f42
@code4rena-admin code4rena-admin changed the title Denial of service Error in modifier ifAdmin (using older versions of Openzeppelin Upgradable Contracts), affected by possible collision of function signature Sep 7, 2023
@raymondfam
Copy link

QA at best.

@c4-pre-sort
Copy link

raymondfam marked the issue as low quality report

@c4-pre-sort c4-pre-sort added the low quality report This report is of especially low quality label Sep 10, 2023
@c4-pre-sort
Copy link

raymondfam marked the issue as primary issue

@c4-pre-sort c4-pre-sort added the primary issue Highest quality submission among a set of duplicates label Sep 10, 2023
@c4-judge
Copy link
Contributor

kirk-baird changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Sep 19, 2023
@c4-judge
Copy link
Contributor

kirk-baird marked the issue as grade-c

@c4-judge c4-judge added grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Sep 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue edited-by-warden grade-c low quality report This report is of especially low quality primary issue Highest quality submission among a set of duplicates QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax unsatisfactory does not satisfy C4 submission criteria; not eligible for awards
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@code423n4 @captainmangoC4 @raymondfam @c4-judge @c4-pre-sort