Hawk High
First Flight #39
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: low
Likelihood: low
Invalid

UUPS upgradeable contracts would benefit from use of OpenZeppelin OwnableUpgradeable contract making the contract more robust

cyfe45

Summary

Although the current implementation of the UUPSUpgradeable contract is not vulnerable to the attacks described in the previous findings, it is recommended to use the OpenZeppelin OwnableUpgradeable contract to make the UUPSUpgradeable contract more robust.

Vulnerability Details

The current implementation makes use of modifiers to ensure the correct ownership and roles of the contract. However, as this implementation does not make use of the heavily audited OwnableUpgradeable contract, it increases the audit surface area and could make the contract vulnerable to attacks.

Benefits of using the OwnableUpgradeable contract:

  • It provides a more robust implementation of the Ownable contract, which includes additional functionality such as the ability to renounce ownership, pause the contract, and more.

  • It has been heavily audited, which means that it has been thoroughly tested and has a proven track record of security.

  • Standard patterns reduce onboarding time for new devs and auditors.

  • Integrates cleanly with other OpenZeppelin contracts.

  • Upgradability-friendly patterns built-in, as it tracks ownership in storage slot compatible with proxies and guards against initialisation collisions.

  • Avoid reinventing the wheel.

Impact

Impact Classification: Low - Informational as this is a recommendation to use the OpenZeppelin OwnableUpgradeable contract.

Likelihood Classification: Low - Likelihood is low as the onlyPrincipal modifier is working.

Tools Used

  • Manual review and research of the OpenZeppelin UUPSUpgradeable contract

Recommendations

  • Use the OpenZeppelin OwnableUpgradeable contract to make the UUPSUpgradeable contract more robust.

+ import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
​
- contract LevelOne is Initializable, UUPSUpgradeable {
+ contract LevelOne is Initializable, UUPSUpgradeable, OwnableUpgradeable {
using SafeERC20 for IERC20;
​
////////////////////////////////
///// /////
///// ERRORS /////
///// /////
////////////////////////////////
- error HH__NotPrincipal();
​
////////////////////////////////
///// /////
///// MODIFIERS /////
///// /////
////////////////////////////////
- modifier onlyPrincipal() {
- if (msg.sender != principal) {
- revert HH__NotPrincipal();
- }
- _;
- }
​
////////////////////////////////
///// /////
///// INITIALIZER /////
///// /////
////////////////////////////////
function initialize(address _principal, uint256 _schoolFees, address _usdcAddress) public initializer {
if (_principal == address(0)) {
revert HH__ZeroAddress();
}
if (_schoolFees == 0) {
revert HH__ZeroValue();
}
if (_usdcAddress == address(0)) {
revert HH__ZeroAddress();
}
​
principal = _principal;
schoolFees = _schoolFees;
usdc = IERC20(_usdcAddress);
​
__UUPSUpgradeable_init();
+ __Ownable_init(_principal);
}
...
​
+ // Also updating the `onlyPrincipal` modifier (where used) with `onlyOwner` modifier instead.
}
Updates

Lead Judging Commences

yeahchibyke Lead Judge 22 days ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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