Hawk High
First Flight #39
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: low
Valid

Costly Operation: For Loop in `LevelOne::removeTeacher` Becomes Inefficient as listOfTeachers Grows

wisedusk501

Summary

The removeTeacher function in LevelOne uses a linear search on an array, which leads to increased gas costs as the number of teachers grows. This inefficiency can affect usability and scalability in production deployments.

Vulnerability Details

removeTeacher uses a loop over listOfTeachers, causing higher gas costs as the list grows.

Impact

Gas cost increases significantly with the number of teachers.

Tools Used

I found this issue through a manual audit.

Proof of Concept:
Here is a test that compares gas usage for removing a teacher from a list of 10 vs 100 teachers.

function test_remove_teacher_gas_cost() public {
// Add 10 teachers
for (uint256 i = 0; i < 10; i++) {
address teacher = makeAddr(string(abi.encodePacked("t10_", vm.toString(i))));
vm.prank(principal);
levelOneProxy.addTeacher(teacher);
}
​
address t10 = makeAddr("t10_0");
​
uint256 gasBefore10 = gasleft();
vm.prank(principal);
levelOneProxy.removeTeacher(t10);
uint256 gasUsed10 = gasBefore10 - gasleft();
​
// Add 100 teachers
for (uint256 i = 0; i < 100; i++) {
address teacher = makeAddr(string(abi.encodePacked("t100_", vm.toString(i))));
vm.prank(principal);
levelOneProxy.addTeacher(teacher);
}
​
address t100 = makeAddr("t100_0");
​
uint256 gasBefore100 = gasleft();
vm.prank(principal);
levelOneProxy.removeTeacher(t100);
uint256 gasUsed100 = gasBefore100 - gasleft();
​
console2.log("Gas used for removing teacher from 10-person list:", gasUsed10);
console2.log("Gas used for removing teacher from 100-person list:", gasUsed100);
}
Gas used for removing teacher from 10-person list: 6306
Gas used for removing teacher from 100-person list: 9942

Recommendations

Use a mapping(address => uint256) to store the index of each teacher. This allows O(1) lookup and removal by swapping with the last element and popping the array.

Add this state variable:

+ mapping(address => uint256) private teacherIndex;

Update LevelOne::addTeacher to store the index:

+ teacherIndex[_teacher] = listOfTeachers.length - 1;

Modify LevelOne::removeTeacher for efficient removal:

- uint256 teacherLength = listOfTeachers.length;
- for (uint256 n = 0; n < teacherLength; n++) {
- if (listOfTeachers[n] == _teacher) {
- listOfTeachers[n] = listOfTeachers[teacherLength - 1];
- listOfTeachers.pop();
- break;
- }
- }
-
- isTeacher[_teacher] = false;
+ uint256 index = teacherIndex[_teacher];
+ uint256 lastIndex = listOfTeachers.length - 1;
+
+ if (index != lastIndex) {
+ address lastTeacher = listOfTeachers[lastIndex];
+ listOfTeachers[index] = lastTeacher;
+ teacherIndex[lastTeacher] = index;
+ }
+
+ listOfTeachers.pop();
+ isTeacher[_teacher] = false;
+ delete teacherIndex[_teacher];
Updates

Lead Judging Commences

yeahchibyke Lead Judge about 1 month ago
Submission Judgement Published
Validated
Assigned finding tags:

possible DoS when removing teachers

Unbounded loops in teacher lists could result in high gas usage when trying to remove a teacher when teachers are plenty. This could result in a possible DoS

Support

FAQs

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