First Flight #6: Voting Booth
First Flight #6
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Invalid

Missing Access Control Modifier in Vote Function (Code Quality Issue)

imth3ak

Summary

The vote function does not use a modifier to check that the voter is allowed and has not voted before. This could make the code less readable and reusable, as the require statement is repeated in the function body.

Vulnerability Details

The vote function is responsible for casting votes for the proposal. However, it does not use a modifier to check that the voter is allowed and has not voted before. Instead, it uses a require statement in the function body, which checks that the voter has the ALLOWED state in the s_voters mapping. This could make the code less readable and reusable, as the require statement is repeated in the function body. It could also increase the gas cost of the function, as the require statement is executed every time the function is called.

A better practice is to use a modifier to check the access control conditions and apply it to the function. This way, the code is more readable and reusable, as the modifier can be applied to other functions that need the same access control. It could also reduce the gas cost of the function, as the modifier is executed only once before the function is called.

Impact

The impact of this issue is that the code quality and readability of the vote function is reduced. This could make the code harder to maintain and debug, as the require statement is repeated in the function body. It could also increase the gas cost of the function, as the require statement is executed every time the function is called.

Tools Used

  • Manual

Test Case

The following test case demonstrates how the vote function works without a modifier:

  • Deploy the contract with an array of three allowed voters and 1 ether as the reward.

  • Call the vote function from one of the allowed voters with a valid input.

  • Observe that the vote is recorded and the state is updated.

  • Call the vote function from the same voter again with a different input.

  • Observe that the transaction reverts with the error message “DP: voter not allowed or already voted”.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.23;
​
import "hardhat/console.sol";
import "./VotingBooth.sol";
​
contract VotingBoothTest {
VotingBooth votingBooth;
​
function testVote() public {
// deploy the contract with three allowed voters and 1 ether as the reward
address[] memory allowList = new address;
allowList[0] = 0x1234567890123456789012345678901234567890;
allowList[1] = 0x2345678901234567890123456789012345678901;
allowList[2] = 0x3456789012345678901234567890123456789012;
votingBooth = new VotingBooth{value: 1 ether}(allowList);
​
// check the initial state of the contract
console.log("Initial state:");
console.log("Total allowed voters: %s", votingBooth.s_totalAllowedVoters());
console.log("Total current votes: %s", votingBooth.s_totalCurrentVotes());
console.log("Voting complete: %s", votingBooth.s_votingComplete());
​
// call the vote function from one of the allowed voters with a valid input
votingBooth.vote{from: allowList[0]}(true);
​
// check the updated state of the contract
console.log("Updated state:");
console.log("Total allowed voters: %s", votingBooth.s_totalAllowedVoters());
console.log("Total current votes: %s", votingBooth.s_totalCurrentVotes());
console.log("Voting complete: %s", votingBooth.s_votingComplete());
​
// call the vote function from the same voter again with a different input
votingBooth.vote{from: allowList[0]}(false); // this should revert
}
}
​

Logs

Initial state:
Total allowed voters: 3
Total current votes: 0
Voting complete: false
Updated state:
Total allowed voters: 3
Total current votes: 1
Voting complete: false
Transaction reverted: DP: voter not allowed or already voted
​

Recommendations

Use a modifier to check that the voter is allowed and has not voted before and apply it to the vote function. This way, the code is more readable and reusable, as the modifier can be applied to other functions that need the same access control. It could also reduce the gas cost of the function, as the modifier is executed only once before the function is called.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.23;
​
import "./Math.sol";
​
contract VotingBooth {
// ... omitted for brevity
​
// modifier to check that the voter is allowed and has not voted before
modifier onlyAllowedVoter() {
// current voter
address voter = msg.sender;
​
// require that the voter is allowed and has not voted before
require(s_voters[voter] == ALLOWED, "DP: voter not allowed or already voted");
​
// continue execution
_;
}
​
// record a vote
function vote(bool voteInput) external onlyAllowedVoter { // apply the modifier
// prevent voting if already completed
require(isActive(), "DP: voting has been completed on this proposal");
​
// update storage to record that this user has voted
s_voters[msg.sender] = VOTED; // use msg.sender instead of voter
​
// ... omitted for brevity
}
​
// ... omitted for brevity
}
​
Updates

Lead Judging Commences

0xnevi Lead Judge
over 1 year ago
0xnevi Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

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