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

Potential DoS when trying to expel students when student count is too high.

bowtiedbrothers

Summary

Due to the way the list of students is looped through when using the 'expel()' function, if there are a large number of students that enroll in the school session the function may not be able (or may not be feasible) to be called due to high gas cost. This could allow an attacker to enroll with many accounts if the enroll fee is too low and can prevent them from being expelled for breaking any rules.

Vulnerability Details

Within the 'expel()' function, the way the list of students is iterated through is not optimal so there is increasing gas cost and complexity as the number of students increases, reaching a point where the gas cost to use the function will be extraordinarily high:

function expel(address _student) public onlyPrincipal {
if (inSession == false) {
revert();
}
if (_student == address(0)) {
revert HH__ZeroAddress();
}
​
if (!isStudent[_student]) {
revert HH__StudentDoesNotExist();
}
​
uint256 studentLength = listOfStudents.length;
for (uint256 n = 0; n < studentLength; n++) {
if (listOfStudents[n] == _student) {
listOfStudents[n] = listOfStudents[studentLength - 1];
listOfStudents.pop();
break;
}
}

Impact

This could allow for an attacker to not be punished for bad behavior/breaking the rules if the list of students is too high, breaking a key invariant.

Tools Used

Manual review and Foundry

Recommendations

Assuming the protocol does not want a large number of students per given session, a simple solution would be to implement a limit in the number of students that could be enrolled per session so that gas costs aren't allowed to get too high for the 'expel()' function to be called:

function enroll() external notYetInSession {
if (isTeacher[msg.sender] || msg.sender == principal) {
revert HH__NotAllowed();
}
if (isStudent[msg.sender]) {
revert HH__StudentExists();
}
​
+ if (listOfStudents.length >= MAX_STUDENTS) {
+ revert HH__MaxStudentsReached();
+ }
usdc.safeTransferFrom(msg.sender, address(this), schoolFees);
​
listOfStudents.push(msg.sender);
isStudent[msg.sender] = true;
studentScore[msg.sender] = 100;
bursary += schoolFees;
​
emit Enrolled(msg.sender);
}

If having a max on the number of students that can enroll per session is not a feasible solution for the protocol, they would need to change the way in which they iterate through the list by instead using an index.

Updates

Lead Judging Commences

yeahchibyke Lead Judge 3 months ago
Submission Judgement Published
Invalidated
Reason: Too generic
yeahchibyke Lead Judge 3 months ago
Submission Judgement Published
Invalidated
Reason: Too generic

Support

FAQs

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