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

`graduateAndUpgrade` does not upgrade the contract

bavoen

Summary

The graduateAndUpgrade function in the LevelOne contract does not upgrade the contract as expected. The function calls the _authorizeUpgrade function from the UUPSUpgradeable contract, but this does not actually upgrade the contract. This function is only used by the UUPSUpgradeable contract to validate the authorization of the upgrade.

The correct way to upgrade the contract is to call the upgradeToAndCall function from the UUPSUpgradeable contract. This function will call the _authorizeUpgrade function to validate the authorization of the upgrade, validate the new implementation, and then upgrade the contract to the new implementation (if the validation is successful).

Vulnerability Details

The vulnerability is located in the graduateAndUpgrade function of the LevelOne contract.

function graduateAndUpgrade(address _levelTwo, bytes memory) public onlyPrincipal {
if (_levelTwo == address(0)) {
revert HH__ZeroAddress();
}
​
uint256 totalTeachers = listOfTeachers.length;
​
uint256 payPerTeacher = (bursary * TEACHER_WAGE) / PRECISION;
uint256 principalPay = (bursary * PRINCIPAL_WAGE) / PRECISION;
​
_authorizeUpgrade(_levelTwo);
​
for (uint256 n = 0; n < totalTeachers; n++) {
usdc.safeTransfer(listOfTeachers[n], payPerTeacher);
}
​
usdc.safeTransfer(principal, principalPay);
}

The _authorizeUpgrade function is called to validate the authorization of the upgrade, but it does not actually upgrade the contract. The contract will not be upgraded and the new implementation will not be used. To validate this theory, we can make a change to the LevelTwo contract and check if the change is reflected when interacting with the proxy contract. The change is not reflected, which confirms that the contract is not upgraded.

Impact

The contract does not upgrade to the new implementation when resolving the graduateAndUpgrade function like documented.

Tools Used

Manually reviewed the code and the documentation.

Recommendations

The graduateAndUpgrade function should be changed to call the upgradeToAndCall function from the UUPSUpgradeable contract instead of calling the _authorizeUpgrade function. This will ensure that the contract is upgraded to the new implementation and that the new implementation is used when interacting with the proxy contract.

-function graduateAndUpgrade(address _levelTwo, bytes memory) public onlyPrincipal {
+function graduateAndUpgrade(address _levelTwo, bytes memory data) public onlyPrincipal {
if (_levelTwo == address(0)) {
revert HH__ZeroAddress();
}
​
uint256 totalTeachers = listOfTeachers.length;
​
uint256 payPerTeacher = (bursary * TEACHER_WAGE) / PRECISION;
uint256 principalPay = (bursary * PRINCIPAL_WAGE) / PRECISION;
​
- _authorizeUpgrade(_levelTwo);
+ upgradeToAndCall(_levelTwo, data);
​
for (uint256 n = 0; n < totalTeachers; n++) {
usdc.safeTransfer(listOfTeachers[n], payPerTeacher);
}
​
usdc.safeTransfer(principal, principalPay);
}

This change alone will not fix the issue. The LevelTwo contract must also be changed to implement the UUPSUpgradeable contract (or at least be compatible with it). The UUPSUpgradeable upgrade function will check the new implementation and validate the upgrade, this will fail in the current implementation of the LevelTwo contract.

Updates

Lead Judging Commences

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

failed upgrade

The system doesn't implement UUPS properly.

yeahchibyke Lead Judge 3 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.