Hawk High
First Flight #39
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: medium
Likelihood: medium
Invalid

Looping through teacher array to check for duplicates in LevelOne::removeTeacher is a potential DoS vector, incrementing gas costs for future teacher entry

batblock

Summary

The removeTeacher function in LevelOne has a scalability issue due to its linear search through the listOfTeachers array. As more teachers are added, gas costs increase significantly when removing a teacher. Additionally, there's a logical flaw until the teacher is actually removed, they can still interact with the contract (e.g., submit reviews), potentially leading to unauthorized actions.

Vulnerability Details

The LevelOne::removeTeacher function has a removal checking mechanism that loops through the listOfTeachers array to search for the teacher. However, the longer the LevelOne::listOfTeachers array is, the more time it takes to find and remove the teacher. This means that the time taken will increase and also the gas increased as the number of teachers added. Meanwhile, the teacher can still review the student even though the teacher should have been removed.


Write the below code in `levelOneAndGraduateTest.t.sol` file.

function testAddingMoreTeacherIncreasesRemoveTeacherGasCost() public {
vm.prank(principal);
levelOneProxy.addTeacher(harry);
vm.prank(principal);
levelOneProxy.removeTeacher(harry);
uint256 gasBefore = gasleft();
​
string memory teacherAddress;
address teacherToRemove;
vm.startPrank(principal);
for (uint256 i = 0; i < 100; i++) {
teacherAddress = Strings.toString(i);
teacherToRemove = makeAddr(teacherAddress);
levelOneProxy.addTeacher(teacherToRemove);
}
vm.stopPrank();
​
vm.prank(principal);
levelOneProxy.removeTeacher(teacherToRemove);
uint256 gasAfter = gasleft();
​
console2.log("Gas cost of remove teacher before:", gasBefore );
console2.log("Gas cost of remove teacher afer:", gasAfter);
​
}

Impact

High Gas Costs: As the number of teachers grows, removing a teacher becomes more expensive due to the linear search and shift operations in the array. This can lead to inefficient contract performance and potential transaction failures if gas limits are exceeded.

Unauthorized Actions: Since a teacher is not immediately restricted upon initiation of the removal process, they can still perform actions (like submitting reviews), which violates expected access controls and could lead to trust or integrity issues in the system.

Tools Used

Manual testing

Recommendations

Use a Mapping Instead of Array: Replace the listOfTeachers array with a mapping(address => bool) or a mapping(address => Teacher) structure for constant-time lookup and removal. This avoids expensive iteration and reduces gas costs significantly.

Implement Soft Delete with Access Control: Instead of fully removing the teacher immediately, introduce a isTeacher[teacher] = false

before the loop. This allows you to instantly restrict access to functions (like reviewing) by checking this flag, even before full cleanup.

Updates

Lead Judging Commences

yeahchibyke Lead Judge
3 months ago
yeahchibyke Lead Judge 3 months ago
Submission Judgement Published
Invalidated
Reason: Lack of quality

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.