Description The ManteritsaVoting:announceWinner function allows the contract owner to call it repeatedly. Each time it gets called a health token gets minted to the winner of the vote.
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];
}
}
list = _martenitsaMarketplace.getListing(winnerTokenId);
_healthToken.distributeHealthToken(list.seller, 1);
emit WinnerAnnounced(winnerTokenId, list.seller);
}
Impact Each time the ManteritsaVoting:announceWinner function gets called the vote winner would get a new minted HealthToken. This exploit requires the contract owner and the vote winner to work together making the likelihood of happening low, but the effect it would have to the protocell would be high.
Proof Of Concept
After the vote has ended the contract owner and the vote winner can work together by minting HealthTokens by having the owner call the ManteritsaVoting:announceWinner function repeatedly and having the vote winner collect the tokens.
Place the following test in your test file
function testReplayVoteWinner() public listMartenitsa {
vm.prank(bob);
voting.voteForMartenitsa(0);
vm.warp(block.timestamp + 1 days + 1);
vm.recordLogs();
voting.announceWinner();
voting.announceWinner();
Vm.Log[] memory entries = vm.getRecordedLogs();
address winner = address(uint160(uint256(entries[0].topics[2])));
console.log("winner", winner);
console.log("winner Health Token Balance ", healthToken.balanceOf(winner));
}
console log
[PASS] testReplayVoteWinner() (gas: 513650)
Logs:
winner 0x4f19A0AcB98b7aA30b0138D26e5E5F9634F32F5A
winner Health Token Balance 2000000000000000000
Recommended Mitigation: To ensure that the ManteritsaVoting:announceWinner function can only be called once, we introduce a boolean state variable that keeps track of whether the function has already been invoked. When the function is called the first time, it sets this variable to true, and on subsequent calls, it will check the variable's value and revert if it's already set to true. Here's how you can modify your function to include this logic:
contract MartenitsaToken is ERC721, Ownable {
uint256 private _nextTokenId;
address[] public producers;
mapping(address => uint256) public countMartenitsaTokensOwner;
mapping(address => bool) public isProducer;
mapping(uint256 => string) public tokenDesigns;
+ bool private _winnerAnnounced = false;
event Created(address indexed owner, uint256 indexed tokenId, string indexed design);
function announceWinner() external onlyOwner {
+ require(!_winnerAnnounced, "Winner already announced");
require(block.timestamp >= startVoteTime + duration, "The voting is active");
+ _winnerAnnounced = true; // Prevent the function from being called again
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];
}
}
list = _martenitsaMarketplace.getListing(winnerTokenId);
_healthToken.distributeHealthToken(list.seller, 1);
emit WinnerAnnounced(winnerTokenId, list.seller);
}
}
