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

Potential DoS via gas consumption on `LevelOne::removeTeacher` function due to unbounded for loop.

blackgrease

Summary

In Hawk High, due to not having a limit on the maximum number of employed teachers, the LevelOne::removeTeacher function is a potential vector for Denial of Service either running out of gas or becoming infeasible to remove a teacher due to gas cost.

Description

The Hawk High LevelOne contract is potentially vulnerable to a Denial of Service attack via an unbounded array in the LevelOne::removeTeacher function.

The Code Issue

The LevelOne:removeTeacher function is used by the Principal to remove a teacher from Hawk High employment. The function iterates over a for loop that is unbounded. It is unbounded because there is no set maximum limit to the number of teachers that can be employed at Hawk High.

Therefore, for a large number of employed teachers, it becomes gas expensive for the Principal to remove a teacher, especially if the teacher is indexed at the end of the listOfTeachers array.

When there are many teachers enrolled, the function may either

  1. consume all gas when run causing a Denial of service,

  2. become unfeasible to remove a teacher due to high gas cost

In either case, the intended functionality is broken.

Affected Code

​
function removeTeacher(address _teacher) public onlyPrincipal {
if (_teacher == address(0)) {
revert HH__ZeroAddress();
}
​
if (!isTeacher[_teacher]) {
revert HH__TeacherDoesNotExist();
}
​
uint256 teacherLength = listOfTeachers.length;//@audit: if this is large, potential DoS vector
for (uint256 n = 0; n < teacherLength; n++) {
if (listOfTeachers[n] == _teacher) {
listOfTeachers[n] = listOfTeachers[teacherLength - 1];
listOfTeachers.pop();
break;
}
}
​
isTeacher[_teacher] = false;
​
emit TeacherRemoved(_teacher);
}

Impact

The severity of this bug is Medium as there is no impact to any funds, directly or indirectly. However, it breaks the expected functionality of being able to remove a teacher from the employment and salary structure in a feasible manner.

When there are many teachers enrolled, the function may:

  1. consume all gas when run causing a Denial of service, failing to remove a teacher. A teacher may still get paid even though they are not employed and still use teacher related functions in the contract.

  2. become unfeasible to remove a teacher due to high gas costs causing the same above issues.

Tools Used

  1. Manual Review

  2. Foundry

Proof of Concept

Description

I have created a runnable Proof-of-Concept that deploys two instances of Hawk High - Level One. One instance has a few teachers employed (10), while the second instance has a larger number of employed teachers students (500). The gas expense of removing a teacher from each are compared to show that it is more expensive to remove a teacher when more are employed.

Note: if multiple teachers are to be expelled, this is an even more gas expensive operation.

Running: forge test --mt testPotentialDoSOnRemovingTeachers -vvv

Code

​
//SPDX-License-Identifier: MIT
​
pragma solidity 0.8.26;
​
import {Test, console2} from "forge-std/Test.sol";
import {LevelOne} from "src/LevelOne.sol";
import {DeployLevelOne} from "script/DeployLevelOne.s.sol";
import {MockUSDC} from "test/mocks/MockUSDC.sol";
​
contract DoSByStudentRemoval is Test {
DeployLevelOne deployBot;
DeployLevelOne deployBot_2;
LevelOne levelOneProxy;
LevelOne levelOneProxy_2;
MockUSDC usdc;
MockUSDC usdc2;
​
address proxyAddress;
address proxyAddress_2;
address levelOneImplementationAddress;
​
//Roles
address principal;
address principal_2;
address[] teachersFew;
address[] teachersMany;
​
//Misc
uint256 schoolFees;
uint256 schoolFees_2;
​
function setUp() public {
//First Instance
deployBot = new DeployLevelOne();
proxyAddress = deployBot.deployLevelOne();
levelOneProxy = LevelOne(proxyAddress);
​
usdc = deployBot.getUSDC(); //used in both instances
principal = deployBot.principal();
schoolFees = deployBot.getSchoolFees();
//Second Instance
deployBot_2 = new DeployLevelOne();
proxyAddress_2 = deployBot_2.deployLevelOne();
levelOneProxy_2 = LevelOne(proxyAddress_2);
​
usdc2 = deployBot_2.getUSDC();
principal_2 = deployBot_2.principal();
schoolFees_2 = deployBot_2.getSchoolFees();
}
​
​
​
modifier startSession() {
vm.startPrank(principal);
levelOneProxy.startSession(5);
_;
}
​
modifier startSession2() {
vm.startPrank(principal_2);
levelOneProxy_2.startSession(5);
_;
}
​
modifier addFewTeachers() {
//Adding 10 teachers
uint256 numberOfTeachers = 10;
teachersFew = new address[](numberOfTeachers);
for (uint256 x = 1; x < numberOfTeachers; x++) {
teachersFew[x] = address(uint160(x));
vm.startPrank(principal);
levelOneProxy.addTeacher(teachersFew[x]);
}
_;
}
​
modifier addManyTeachers() {//Using second deployment of school for comparison
//Adding 500 teachers
uint256 numberOfTeachers = 500;
teachersMany = new address[](numberOfTeachers);
for (uint256 x = 1; x < numberOfTeachers; x++) {
teachersMany[x] = address(uint160(x));
vm.startPrank(principal);
levelOneProxy_2.addTeacher(teachersMany[x]);
}
_;
}
​
function GasConsumptionOnFewTeachers() internal addFewTeachers startSession returns (uint256) {
address teacherToExpel = teachersFew[teachersFew.length - 1]; //the last index in the array making the loop run until the end
uint256 startingGas = gasleft();
​
vm.startPrank(principal);
levelOneProxy.removeTeacher(teacherToExpel);
​
uint256 remainingGas = gasleft();
uint256 gasConsumption = startingGas - remainingGas;
​
return gasConsumption;
}
​
function GasConsumptionOnManyTeachers() internal addManyTeachers startSession2 returns (uint256) {
address teacherToExpel = teachersMany[teachersMany.length - 1]; //the last index in the array making the loop run until the end
uint256 startingGas = gasleft();
​
vm.startPrank(principal_2);
levelOneProxy_2.removeTeacher(teacherToExpel);
​
uint256 remainingGas = gasleft();
uint256 gasConsumption = startingGas - remainingGas;
​
return gasConsumption;
}
​
function testPotentialDoSOnRemovingTeachers() public {
​
uint256 gasConsumptionFewTeachers = GasConsumptionOnFewTeachers();
uint256 gasConsumptionManyTeachers = GasConsumptionOnManyTeachers();
​
uint256 gasDifference = gasConsumptionManyTeachers - gasConsumptionFewTeachers;
​
uint256 numberOfTeachers = levelOneProxy.getTotalTeachers();
uint256 numberOfTeachers_2 = levelOneProxy_2.getTotalTeachers();
​
// //Logging
console2.log("Number of teachers in first deployment: ", numberOfTeachers);
console2.log("Number of teachers in second deployment: ", numberOfTeachers_2);
console2.log("Gas used for few teachers: ", gasConsumptionFewTeachers);
console2.log("Gas used for many teachers: ", gasConsumptionManyTeachers);
console2.log("Gas Difference: ", gasDifference);
}
}
​
​
​

Mitigation

A recommended mitigation to this, is to set a reasonable maximum limit to the number of employed teachers. For flexibility, the principal can adjust the number of allowed teachers depending on supply and demand.

Updates

Lead Judging Commences

yeahchibyke Lead Judge 29 days ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

spam

Appeal created

blackgrease Submitter
28 days ago
yeahchibyke Lead Judge 26 days 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

spam

Support

FAQs

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