Summary
In file LevelOne.sol function graduateAndUpgrade()
It is possible to upgrade the contract even if the session is active and not reaching the session end time
Vulnerability Details
In the graduateAndUpgrade function, there's no check if the sessionEnd time is reached.
A principle can perform upgrade with no sessionEnd requirement
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);
}
Proof of Concept
Add the following test within test/LeveOnelAndGraduateTest.t.sol
function test_confirm_can_graduate_before_session_ends()
public
schoolInSession
{
levelTwoImplementation = new LevelTwo();
levelTwoImplementationAddress = address(levelTwoImplementation);
bytes memory data = abi.encodeCall(LevelTwo.graduate, ());
assertTrue(
levelOneProxy.getSessionStatus(),
"Session should be in progress"
);
assertLt(
block.timestamp,
levelOneProxy.getSessionEnd(),
"Time should be before sessionEnd"
);
vm.warp(block.timestamp + 1 weeks);
vm.prank(principal);
levelOneProxy.graduateAndUpgrade(levelTwoImplementationAddress, data);
}
Outputs
forge test -vvv --match-test test_confirm_can_graduate_before_session_ends
[⠊] Compiling...
No files changed, compilation skipped
Ran 1 test for test/LeveOnelAndGraduateTest.t.sol:LevelOneAndGraduateTest
[PASS] test_confirm_can_graduate_before_session_ends() (gas: 1174058)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 13.16ms (2.27ms CPU time)
Ran 1 test suite in 132.04ms (13.16ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Impact
Invariant breaking -- System upgrade cannot take place unless school's sessionEnd has reached
Tools Used
Manual review
Recommendations
Considering adding the time check before perform upgrades
modifier onlyAfterSession() {
if (!inSession || block.timestamp < sessionEnd) {
revert HH__SessionStillAlive();
}
_;
}
function graduateAndUpgrade(address _levelTwo, bytes memory)
public
onlyPrincipal
onlyAfterSession
{
…
_authorizeUpgrade(_levelTwo);
…
}
