DittoETH
Ditto
DeFiFoundryOracle
55,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Wrong implementation of `onlyAdminOrDAO() modifier`

nadin

Summary

modifier onlyAdminOrDAO() is used in setter functions in the OwnerFacet.sol contract, however the current implementation is incorrect and prevents the functionality from working as expected.

Vulnerability Details

  • Take a look at modifier onlyAdminOrDAO() : here

65 modifier onlyAdminOrDAO() {
66 if (msg.sender != LibDiamond.contractOwner() && msg.sender != s.admin) { <---- should uses `||` instead of `&&`
67 revert Errors.NotOwnerOrAdmin();
68 }
69 _;
70 }

  • modifier onlyAdminOrDAO() is used to grant permissions to onlyAdmin or DAO in some setter functions in contracts OwnerFacet.sol. However as we see the current implementation uses && instead of || causing this function to not work as expected.

Impact

Current implementation may break functionality

Tools Used

Manual review

Recommendations

Use || instead of && in modifier onlyAdminOrDAO()

--- if (msg.sender != LibDiamond.contractOwner() && msg.sender != s.admin) {
+++ if (msg.sender != LibDiamond.contractOwner() || msg.sender != s.admin) {
Updates

Lead Judging Commences

0xnevi Lead Judge
almost 2 years ago
0xnevi Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.