Summary
The LevelOne::graduateAndUpgrade function call _authorizeUpgrade(_levelTwo); to upgrade the contract to LevelTwo. But the signature of the function graduateAndUpgrade(address _levelTwo, bytes memory) does pass an unnamed bytes data parameter, which is not used ans should hold LevelTwo::graduate function. This means that the intended behavior is to call upgradeToAndCall UUPS function instead of _authorizeUpgrade(_levelTwo);. Or the deployment script GraduateToLevelTwo should call upgradeToAndCall right after LevelOne::graduateAndUpgrade. However, this is not the case in the current implementation and the LevelTwo contract is not initialized.
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);
}
function run() external returns (address levelTwoImplementationAddress) {
vm.startBroadcast();
LevelTwo levelTwo = new LevelTwo();
vm.stopBroadcast();
levelTwoImplementationAddress = address(levelTwo);
LevelOne levelOneProxy = LevelOne(levelOneProxyAddress);
bytes memory data = abi.encodeCall(LevelTwo.graduate, ());
vm.startBroadcast();
@> levelOneProxy.graduateAndUpgrade(levelTwoImplementationAddress, data);
vm.stopBroadcast();
return levelTwoImplementationAddress;
}
Impact
Leaving the LevelTwo contract uninitialized can lead to a situation where the contract is not properly set up and can lead to unexpected behavior. The LevelTwo contract will not be able to function as intended and can lead to potential issues with the protocol.
Proof of Concept
Principal calls the LevelOne::graduateAndUpgrade function with the LevelTwo implementation address and the LevelTwo::graduate calldata parameter.
We expect an event to be emitted, following ERC1967, with the IERC1967::Upgraded event and the LevelTwo implementation address.
Event is not received which means the LevelTwo contract is not initialized without calling the LevelTwo::graduate function.
See below the test case for the PoC and logs:
function test_audit_level_two_is_not_initialized_even_with_graduate_call_data() public schoolInSession {
levelTwoImplementation = new LevelTwo();
levelTwoImplementationAddress = address(levelTwoImplementation);
bytes memory data = abi.encodeCall(LevelTwo.graduate, ());
vm.prank(principal);
vm.expectEmit();
emit IERC1967.Upgraded(levelTwoImplementationAddress);
levelOneProxy.graduateAndUpgrade(levelTwoImplementationAddress, data);
}
├─ [0] VM::prank(principal: [0x6b9470599cb23a06988C6332ABE964d6608A50ca])
│ └─ ← [Return]
├─ [0] VM::expectEmit()
│ └─ ← [Return]
├─ emit Upgraded(implementation: LevelTwo: [0x2e234DAe75C793f67A35089C9d99245E1C58470b])
├─ [84129] ERC1967Proxy::fallback(LevelTwo: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 0xd3618cca)
│ ├─ [83640] LevelOne::graduateAndUpgrade(LevelTwo: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 0xd3618cca) [delegatecall]
│ │ ├─ [25750] MockUSDC::transfer(first_teacher: [0xeeEeC5A3afd714e3C63A0b1ef6d80722Bcc514b3], 10500000000000000000000 [1.05e22])
│ │ │ ├─ emit Transfer(from: ERC1967Proxy: [0x90193C961A926261B756D1E5bb255e67ff9498A1], to: first_teacher: [0xeeEeC5A3afd714e3C63A0b1ef6d80722Bcc514b3], value: 10500000000000000000000 [1.05e22])
│ │ │ └─ ← [Return] true
│ │ ├─ [25750] MockUSDC::transfer(second_teacher: [0xb4c265c1f1d07474E3715F65724E8fa9d662BF0e], 10500000000000000000000 [1.05e22])
│ │ │ ├─ emit Transfer(from: ERC1967Proxy: [0x90193C961A926261B756D1E5bb255e67ff9498A1], to: second_teacher: [0xb4c265c1f1d07474E3715F65724E8fa9d662BF0e], value: 10500000000000000000000 [1.05e22])
│ │ │ └─ ← [Return] true
│ │ ├─ [25750] MockUSDC::transfer(principal: [0x6b9470599cb23a06988C6332ABE964d6608A50ca], 1500000000000000000000 [1.5e21])
│ │ │ ├─ emit Transfer(from: ERC1967Proxy: [0x90193C961A926261B756D1E5bb255e67ff9498A1], to: principal: [0x6b9470599cb23a06988C6332ABE964d6608A50ca], value: 1500000000000000000000 [1.5e21])
│ │ │ └─ ← [Return] true
│ │ └─ ← [Stop]
│ └─ ← [Return]
└─ ← [Revert] log != expected log
Tools Used
Manual review.
Recommendations
Ensure the deployment script calls upgradeToAndCall(_levelTwo, data); after LevelOne::graduateAndUpgrade to properly initialize the LevelTwo contract. Or update the LevelOne::graduateAndUpgrade function to call upgradeToAndCall(_levelTwo, data); instead of _authorizeUpgrade(_levelTwo);. This will ensure that the LevelTwo contract is properly initialized and can function as intended.
- 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);
}
