Description: In graduateAndUpgrade function, it only calls _authorizeUpgrade(_levelTwo),
missing _upgradeToAndCallUUPS for the proxy to upgrade to LevelTwo contract.
Impact: principal will still need to call upgradeToAndCall to upgrade the proxy to LevelTwo contract after calling graduateAndUpgrade
Proof of Concept: add following test and run
function readImplementation(address proxy) public returns (address implementation) {
bytes32 implSlot = bytes32(uint256(keccak256("eip1967.proxy.implementation")) - 1);
bytes32 raw = vm.load(proxy, implSlot);
implementation = address(uint160(uint256(raw)));
}
...
function test_graduateAndUpgrade_wont_upgrade() public {
address implementationBefore = readImplementation(address(levelOneProxy));
vm.startPrank(principal);
LevelTwo levelTwo = new LevelTwo();
bytes memory data = abi.encodeCall(LevelTwoEdit.graduate, ());
levelOneProxy.graduateAndUpgrade(address(levelTwo), data);
vm.stopPrank();
address implementationAfter = readImplementation(address(levelOneProxy));
assertEq(implementationBefore, implementationAfter);
}
Recommended Mitigation:
Add _upgradeToAndCallUUPS to the graduateAndUpgrade function, or directly call upgradeToAndCall in the graduateAndUpgrade function
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);
+ upgradeToAndCall(_levelTwo, abi.encodeWithSignature("graduate()"));
}