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

The `LevelOne::graduateAndUpgrade` function does not upgrade the proxy to a new implementation

arinthal

Vulnerability Details

The LevelOne::graduateAndUpgrade function is intended to upgrade the proxy to a newer implementation. According to the OpenZeppelin documentation (https://docs.openzeppelin.com/contracts/5.x/api/proxy#UUPSUpgradeable), upgrading a UUPS proxy requires calling the upgradeToAndCall function imported from the UUPSUpgradeable contract. In this case, that call should be made from within the LevelOne contract. However, as demonstrated in the PoC below, the proxy's implementation is not actually upgraded.

Additionally, the LevelTwo contract, which contains the new implementation, should comply with the EIP-1967 standard — but unfortunately, it does not.

Impact

The proxy implementation cannot be upgraded using the LevelOne::graduateAndUpgrade function.

Proof of Code

Add the following code to the LevelOneAndGraduateTest.t.sol file within the LevelOneAndGraduateTest contract.

function test_confirm_can_upgrade() public schoolInSession {
levelTwoImplementation = new LevelTwo();
levelTwoImplementationAddress = address(levelTwoImplementation);
​
bytes memory data = abi.encodeCall(LevelTwo.graduate, ());
​
bytes32 IMPLEMENTATION_SLOT = 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc;
​
bytes32 oldImpl = vm.load(proxyAddress, IMPLEMENTATION_SLOT);
address oldImplAddr = address(uint160(uint256(oldImpl)));
​
vm.prank(principal);
levelOneProxy.graduateAndUpgrade(levelTwoImplementationAddress, data);
​
bytes32 newImpl = vm.load(proxyAddress, IMPLEMENTATION_SLOT);
address newImplAddr = address(uint160(uint256(newImpl)));
​
// equal implementation addresses proves, that implementation hasn't upgraded properly
assertEq(newImplAddr, oldImplAddr);
//new implementation address is not equal to deployed LevelTwo contract address
assertNotEq(newImplAddr, levelTwoImplementationAddress);
}

According to the docs (https://docs.openzeppelin.com/contracts/5.x/api/proxy#UUPSUpgradeable), the bytes32 IMPLEMENTATION_SLOT holds the address of the current implementation contract. This value should change when the implementation is upgraded. However, in this case, the value in the slot remains unchanged, indicating that the upgrade did not occur.

Tools Used

  • Manual Review

  • Foundry

Recommended Mitigation

To upgrade the current implementation of a UUPSUpgradeable contract to a newer version, the upgradeToAndCall function should be called from the current implementation contract, passing the new implementation address and, optionally, additional initialization data as parameters. This can be done as demonstrated in the code below.

We need to update the code below in the LevelOne.sol file.

function graduateAndUpgrade(
address _levelTwo,
- bytes memory data
+ 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);
}

The new implementation contract should follow the EIP-1967 standard, as shown below.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;
​
+ import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
​
- contract LevelTwo is Initializable {
+ contract LevelTwo is Initializable, UUPSUpgradeable {
using SafeERC20 for IERC20;
​
address principal;
bool inSession;
uint256 public sessionEnd;
uint256 public bursary;
uint256 public cutOffScore;
mapping(address => bool) public isTeacher;
mapping(address => bool) public isStudent;
mapping(address => uint256) public studentScore;
address[] listOfStudents;
address[] listOfTeachers;
​
uint256 public constant TEACHER_WAGE_L2 = 40;
uint256 public constant PRINCIPAL_WAGE_L2 = 5;
uint256 public constant PRECISION = 100;
​
IERC20 usdc;
​
+ error HH__NotPrincipal();
​
+ modifier onlyPrincipal() {
+ if (msg.sender != principal) {
+ revert HH__NotPrincipal();
+ }
+ _;
+ }
​
​
// here will be the contract functions
​
​
+ function _authorizeUpgrade(
+ address newImplementation
+ ) internal override onlyPrincipal {}
}
​

This ensures that the contract can be upgraded to a new version.

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.