Hawk High
First Flight #39
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

H-03: `graduateAndUpgrade` Function Fails to Perform Contract Upgrade

albert

Summary

The graduateAndUpgrade function in LevelOne.sol calls _authorizeUpgrade(_levelTwo) but lacks the necessary call to an actual UUPS upgrade function (e.g., _upgradeToAndCallUUPS). As a result, the contract will not be upgraded to LevelTwo as intended.

Vulnerability Details

The UUPS (Universal Upgradeable Proxy Standard) pattern involves two steps for an upgrade:

  1. Authorization: The current implementation authorizes a new implementation (e.g., via _authorizeUpgrade).

  2. Execution: The proxy contract's upgradeTo or upgradeToAndCall function is called, which then invokes the authorization in the current implementation and, if authorized, changes the proxy's implementation address.

LevelOne.sol#graduateAndUpgrade only performs step 1 implicitly by being onlyPrincipal and calling _authorizeUpgrade. The OZ UUPSUpgradeable contract expects that _authorizeUpgrade is an internal hook, and the upgrade itself is triggered by a function like _upgradeToAndCallUUPS.

// In graduateAndUpgrade:
_authorizeUpgrade(_levelTwo); // This only sets an internal flag or performs checks within the UUPS context.
// It does NOT change the proxy's implementation.

The function is missing a call like _upgradeToAndCallUUPS(_levelTwo, dataForLevelTwoInitialize, false);.

Impact

The contract will not be upgraded to LevelTwo when graduateAndUpgrade is called. Wages might be paid (if H-02 is fixed), but the system will remain on LevelOne. This fundamentally breaks the school's lifecycle of upgrading to a new system after a session, defeating a primary purpose of the contract.

Tools Used

Manual Review, Understanding of OpenZeppelin UUPSUpgradeable mechanics.

Recommendations

Modify graduateAndUpgrade to correctly initiate the upgrade by calling the appropriate internal UUPS upgrade function (e.g., _upgradeToAndCallUUPS) after all checks and wage payments are made.

(The code modification for this is combined with H-02, H-04, and L-03 fixes in the graduateAndUpgrade function shown below H-04.)

Consolidated Code Modification for LevelOne.sol::graduateAndUpgrade (addressing H-02, H-03, H-04, L-03):

// src/LevelOne.sol
// ... (other parts of the contract) ...
​
function graduateAndUpgrade(address _levelTwo, bytes memory dataForLevelTwoInitialize) public onlyPrincipal {
if (_levelTwo == address(0)) {
revert HH__ZeroAddress();
}
​
// --- START OF MODIFICATION FOR H-04 (Invariant Checks) ---
require(inSession, "HH__NotInSession"); // Added: Ensure session was started
require(block.timestamp >= sessionEnd, "HH__SessionNotEnded");
​
for (uint256 i = 0; i < listOfStudents.length; i++) {
address student = listOfStudents[i];
// Assuming H-05 (reviewCount increment) and M-01 (review limit to 4) are fixed.
require(reviewCount[student] == 4, "HH__StudentReviewIncomplete");
}
// Note: The filtering of students who don't meet cutOffScore is handled
// by LevelTwo's reinitializer using the inherited studentScore and cutOffScore.
// LevelOne must pass its cutOffScore to LevelTwo.
// The `dataForLevelTwoInitialize` should include this.
// --- END OF MODIFICATION FOR H-04 ---
​
uint256 totalTeachers = listOfTeachers.length;
uint256 currentBursary = bursary; // Use a temporary variable for calculations
uint256 principalPay = (currentBursary * PRINCIPAL_WAGE) / PRECISION;
​
// --- START OF MODIFICATION FOR H-02 (Correct Teacher Wage Calculation) ---
uint256 totalTeacherShare = (currentBursary * TEACHER_WAGE) / PRECISION;
uint256 payPerTeacher = 0;
if (totalTeachers > 0) {
payPerTeacher = totalTeacherShare / totalTeachers;
}
// --- END OF MODIFICATION FOR H-02 ---
​
// --- START OF MODIFICATION FOR L-03 (Update bursary state variable) ---
// This reflects that 40% (35% teachers + 5% principal) is paid out.
// The remaining 60% stays in the bursary.
bursary = currentBursary - principalPay - totalTeacherShare;
// --- END OF MODIFICATION FOR L-03 ---
​
// Pay teachers
// If payPerTeacher is 0 (no teachers or zero share), loop won't run or transfers 0.
// Dust from division (if totalTeacherShare % totalTeachers != 0) will remain from totalTeacherShare
// and effectively add to the remaining bursary.
for (uint256 n = 0; n < totalTeachers; n++) {
if (payPerTeacher > 0) { // Avoid 0 value transfers if not needed by token
usdc.safeTransfer(listOfTeachers[n], payPerTeacher);
}
}
​
// Pay principal
if (principalPay > 0) { // Avoid 0 value transfers
usdc.safeTransfer(principal, principalPay);
}
// --- START OF MODIFICATION FOR H-03 (Actual Upgrade Call) ---
// _authorizeUpgrade is an internal function and will be called by _upgradeToAndCallUUPS.
// The `onlyPrincipal` modifier on this function already gates who can call it.
// `dataForLevelTwoInitialize` should be abi.encodeWithSignature("reinitializerFunctionName(types...)", args...)
// For example, for LevelTwo's graduate(): abi.encodeWithSignature("graduate(address,address,uint256)", newPrincipal, usdcAddress, currentCutOffScore)
_upgradeToAndCallUUPS(_levelTwo, dataForLevelTwoInitialize, false);
// --- END OF MODIFICATION FOR H-03 ---
​
emit Graduated(_levelTwo); // Event was present in README example, ensure it's emitted
}
​
// _authorizeUpgrade is an override required by UUPSUpgradeable
// It's called by the UUPS mechanism during an upgrade, not directly by us before calling _upgradeToAndCallUUPS.
// The onlyPrincipal modifier on graduateAndUpgrade already ensures only principal can initiate.
function _authorizeUpgrade(address newImplementation) internal override onlyPrincipal {}
​
// ... (other parts of the contract) ...
Updates

Lead Judging Commences

yeahchibyke Lead Judge 6 months ago
Submission Judgement Published
Validated
Assigned finding tags:

failed upgrade

The system doesn't implement UUPS properly.

Support

FAQs

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