First Flight #13: Baba Marta
First Flight #13
Beginner FriendlyFoundryGameFi
100 EXP
View results
Previous Next
Submission Details
Severity: low
Invalid

`MartenitsaVoting::announceWinner` will face a Denial of Service when `_tokenIds` array size is above a certain threshold which makes the txn out of gas

shikhar229169

Summary

On blockchain a transaction has some gaslimit, exceeding which reverts the transaction and it is not executed. A for loop which runs for a certain number of iterations such that the count of iterations exceed the gas limit for that transaction leads to out of gas error and the transaction reverts.

The announceWinner function is implemented in such a way that it iterates all over the _tokenIds array to find the token id having the highest votes received and rewards the winner.

But the length of _tokenIds being larger than the threshold limit corresponding to the gas limit will eventually lead to out of gas scenario and transaction will be reverted and winner can never be announced.

As well as voteForMartenitsa function makes the token ids present in _tokenIds array redundant because when x amount of users votes for the same tokenId, then that token id is added in that array x times.

Vulnerability Details

The vulnerability is present in the announceWinner function due to its implementation of deciding the winner by iterating all over the _tokenIds array to get the winner with the largest votes.

But _tokenIds array getting very large will result in out of gas and thus reverting the whole transaction, as a result of which the transaction can never be executed.

Even though the total amount of unique token ids receiving votes are within the limit, but due to the voteForMartenitsa function adding every token id again and again in it will make it tremendously large, thus leads to a revert.

Impact

  • Winner can never be announced.

  • No health token will be rewarded to the winner.

Tools Used

Manual Review

Recommendations

Instead of iterating over the whole _tokenIds array in announceWinner function, it would be more beneficial to keep the track of current tokenid winning everytime when voteForMartenitsa is called. In this way there will be no requirement to iterate over the whole array, and the O(N) operation is now minimized to just O(1) and DoS due to gas limit exceeding will thus be prevented.

diff --git a/src/MartenitsaVoting.sol b/src/MartenitsaVoting.sol
index 992f49e..81ce157 100644
--- a/src/MartenitsaVoting.sol
+++ b/src/MartenitsaVoting.sol
@@ -14,6 +14,7 @@ contract MartenitsaVoting is Ownable {
uint256 public startVoteTime;
uint256 public duration = 1 days;
uint256[] private _tokenIds;
+ uint256 private highestVotedToken;
//mapping MartenitsaTokenId -> count of votes
mapping(uint256 => uint256) public voteCounts;
@@ -47,7 +48,10 @@ contract MartenitsaVoting is Ownable {
require(list.forSale, "You are unable to vote for this martenitsa");
hasVoted[msg.sender] = true;
voteCounts[tokenId] += 1;
+ if (voteCounts[tokenId] > voteCounts[highestVotedToken]) {
+ highestVotedToken = tokenId;
+ }
_tokenIds.push(tokenId);
}
@@ -57,15 +61,8 @@ contract MartenitsaVoting is Ownable {
function announceWinner() external onlyOwner {
require(block.timestamp >= startVoteTime + duration, "The voting is active");
- uint256 winnerTokenId;
- uint256 maxVotes = 0;
-
- for (uint256 i = 0; i < _tokenIds.length; i++) {
- if (voteCounts[_tokenIds[i]] > maxVotes) {
- maxVotes = voteCounts[_tokenIds[i]];
- winnerTokenId = _tokenIds[i];
- }
- }
+ uint256 winnerTokenId = highestVotedToken;
+ require(voteCounts[winnerTokenId] > 0, "There was no voting!");
list = _martenitsaMarketplace.getListing(winnerTokenId);
_healthToken.distributeHealthToken(list.seller, 1);
Updates

Lead Judging Commences

bube Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Unbounded arrays

shikhar229169 Submitter
over 1 year ago
bube Lead Judge
over 1 year ago
bube Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Unbounded arrays

Support

FAQs

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