Summary
The design of graduateAndUpgrade function is flawed. If we refer to the documentation, upgrade should not occur if:
-
any student has not gotten 4 reviews.
-
any student who doesn't meet the cutOffScore.
-
the sessionEnd has not been reached.
However, the current implementation does not check for these conditions. This can lead to a situation where the upgrade occurs even if the conditions are not met, leading to potential issues with the system.
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);
}
Impact
The graduateAndUpgrade function does not check for the conditions that should be met before the upgrade occurs. This breaks the core invariants of the system and can lead to potential issues with the system.
Proof of Concept
Principal sets the sessionEnd to 4 weeks and the cutOffScore to 90.
Teacher gives review for two weeks for two students out of 6.
Principal graduates and upgrades to LevelTwo and should revert.
The graduateAndUpgrade function is called and passes. Invariant breaks.
See below the test case for the PoC and logs:
modifier auditSchoolInSession() {
_teachersAdded();
_studentsEnrolled();
vm.prank(principal);
levelOneProxy.startSession(90);
_;
}
function test_audit_principal_can_graduate_without_invariants_check() public auditSchoolInSession {
for (uint256 i = 0; i < 3; i++) {
vm.warp(block.timestamp + 1 weeks);
vm.startPrank(alice);
levelOneProxy.giveReview(harriet, false);
levelOneProxy.giveReview(eli, false);
vm.stopPrank();
}
levelTwoImplementation = new LevelTwo();
levelTwoImplementationAddress = address(levelTwoImplementation);
bytes memory data = abi.encodeCall(LevelTwo.graduate, ());
vm.prank(principal);
vm.expectRevert();
levelOneProxy.graduateAndUpgrade(levelTwoImplementationAddress, data);
}
├─ [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] next call did not revert as expected
Tools Used
Manual review.
Recommendations
Protocol should check for core invariants before allowing the upgrade to occur. This can be done by adding checks for the cutOffScore, reviewCount, and sessionEnd variables in the graduateAndUpgrade function. If any of these conditions are not met, the function should revert.
require(
block.timestamp >= sessionEnd,
"Session has not ended yet"
);
require(
reviewCount[_student] == 4,
"Student has not received 4 reviews"
);
require(
studentScore[_student] >= cutOffScore,
"Student does not meet the cut off score"
);
