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

L-08: `_authorizeUpgrade` in `LevelOne` Does Not Need `onlyPrincipal` if Called Correctly by OZ UUPS

albert

Summary

The _authorizeUpgrade function in LevelOne.sol is marked internal override onlyPrincipal {}. While OpenZeppelin's UUPSUpgradeable pattern expects this function to be overridden, the onlyPrincipal modifier on _authorizeUpgrade itself is redundant if the function that triggers the upgrade (e.g., graduateAndUpgrade which calls _upgradeToAndCallUUPS) is already correctly access-controlled by onlyPrincipal. The UUPS mechanism calls _authorizeUpgrade internally.

Vulnerability Details

OpenZeppelin's UUPSUpgradeable.sol has:

function _authorizeUpgrade(address newImplementation) internal virtual {}

And _upgradeToAndCallUUPS (which is called by LevelOne.graduateAndUpgrade) calls this _authorizeUpgrade.

// In UUPSUpgradeable.sol
function _upgradeToAndCallUUPS(
address newImplementation,
bytes memory data,
bool forceCall
) internal {
// ...
_authorizeUpgrade(newImplementation); // Internal call
// ...
}

Since LevelOne.graduateAndUpgrade is onlyPrincipal, and it's the sole entry point in LevelOne that leads to _authorizeUpgrade being called (via _upgradeToAndCallUUPS), the onlyPrincipal on _authorizeUpgrade itself is not strictly necessary for security in this specific contract's flow. The OZ UUPS design relies on the implementer correctly protecting the entry point that calls upgradeTo or _upgradeTo... functions.

Impact

No direct security impact in the current LevelOne.sol contract, as the calling path via graduateAndUpgrade is correctly protected. This is more a matter of understanding the UUPS pattern and avoiding redundant modifiers. In some complex inheritance scenarios, or if there were other (buggy) internal paths to _authorizeUpgrade, it could offer an extra layer, but it's not its primary defense mechanism in UUPS. OpenZeppelin's documentation often shows _authorizeUpgrade without an explicit access modifier within its body, relying on the caller of upgradeTo to be secured.

Tools Used

Manual Review, Analysis of OpenZeppelin UUPSUpgradeable contract.

Recommendations

The onlyPrincipal modifier on _authorizeUpgrade can be safely removed if desired, as the protection is sufficiently provided by the onlyPrincipal modifier on graduateAndUpgrade. However, leaving it in does no harm and aligns with a belt-and-suspenders approach to security, or if the developer prefers to explicitly state the authorization check at the _authorizeUpgrade level itself, as a clear statement of intent for that function's role.

Code Modification for LevelOne.sol::_authorizeUpgrade (Optional Simplification):

// src/LevelOne.sol
// ... (other parts of the contract) ...
​
// Original and still perfectly fine:
// function _authorizeUpgrade(address newImplementation) internal override onlyPrincipal {}
​
// Optional simplification (if relying solely on graduateAndUpgrade's protection):
function _authorizeUpgrade(address newImplementation) internal override {
// --- START OF MODIFICATION FOR L-08 (Optional) ---
// The onlyPrincipal check is effectively handled by the caller graduateAndUpgrade.
// However, explicit check here is also fine as a safeguard.
// If kept, ensure 'principal' is accessible and correct.
// For UUPS, the key is that only an authorized party (here, principal via graduateAndUpgrade)
// can initiate an action that *leads* to _authorizeUpgrade being checked.
// Standard OZ examples often leave this empty or with custom logic not necessarily repeating the caller's auth.
// If the intent is that *only* the principal of LevelOne can ever authorize:
if (msg.sender != principal) { // This check is actually problematic if _authorizeUpgrade is called internally by the contract itself
// during _upgradeToAndCallUUPS, as msg.sender might be the contract.
// The UUPS _authorizeUpgrade is more about *who* is *allowed* to be the new implementation
// or if the *current contract owner* (e.g. OZ OwnableUpgradeable's owner) permits it.
// Given 'principal' is the contract's admin, using onlyPrincipal on graduateAndUpgrade is correct.
// The `onlyPrincipal` modifier on _authorizeUpgrade itself is slightly unconventional for UUPS
// as this hook is called internally.
// A more standard check inside _authorizeUpgrade, if any, would be:
// require(_msgSender() == owner(), "Ownable: caller is not the owner"); if using Ownable.
// But since LevelOne uses its own `principal` state var:
// require(_msgSender() == principal, "HH__NotPrincipal"); // This would be the check if _authorizeUpgrade was called externally.
// Since it's internal during UUPS, this check is effectively done by
// the `onlyPrincipal` on `graduateAndUpgrade`.
}
// If keeping `onlyPrincipal` modifier on the function signature, the body can be empty.
// --- END OF MODIFICATION FOR L-08 ---
}
// Given the way LevelOne is structured, the `onlyPrincipal` modifier on the function signature `_authorizeUpgrade`
// means if it were ever called from a context where msg.sender != principal, it would revert.
// During the UUPS upgrade process, msg.sender in the context of `_authorizeUpgrade`
// will be `address(this)` because it's an internal call from `_upgradeToAndCallUUPS`.
// Therefore, the `onlyPrincipal` modifier as `if (msg.sender != principal)` would FAIL if principal is an EOA.
​
// **Correction/Clarification on L-08 Recommendation:**
// The `onlyPrincipal` modifier on `_authorizeUpgrade` as written in the contract:
// modifier onlyPrincipal() {
// if (msg.sender != principal) { // This `msg.sender` is the external caller to the transaction
// revert HH__NotPrincipal();
// }
// _;
// }
// function _authorizeUpgrade(...) internal override onlyPrincipal {}
// This is fine. It means that if `_authorizeUpgrade` were somehow called in a transaction initiated by someone other than `principal`,
// it would revert. Since `_upgradeToAndCallUUPS` is internal and preserves `msg.sender` context of the original transaction submitter
// (the principal who called `graduateAndUpgrade`), this `onlyPrincipal` on `_authorizeUpgrade` correctly ensures the whole operation
// is principal-authorized. So, no change is strictly needed, and it serves as a correct redundant check.
// The initial assessment that it was "redundant" was slightly misinterpreting how msg.sender propagates in internal UUPS calls.
// It is actually a good safeguard. My apologies for the slight confusion in the initial L-08 rationale.
// The modifier correctly ensures the entire upgrade operation chain is initiated by the principal.
// No change recommended for L-08 based on this refined understanding. It is robust as is.

Revised Stance on L-08:
Upon further reflection on msg.sender context within UUPS internal calls, the onlyPrincipal modifier on _authorizeUpgrade is indeed a valid and robust safeguard, ensuring that the entire transaction leading to the upgrade must have been initiated by the principal. It is not merely redundant but reinforces the security posture. Therefore, no change is recommended for L-08; the current implementation is good. My apologies for the prior suggestion of optional removal.

It seems the most critical issues have been covered. The remaining points are more about hardening, clarity, and adherence to very strict interpretations of invariants or best practices, rather than direct exploit vectors of high severity given the current contract structure.

Updates

Lead Judging Commences

yeahchibyke Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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