General users can change their votes during the last hour

[hats-bug-reporter[bot]]

Github username: @YanhuiJessica Twitter username: -- Submission hash (on-chain): 0x68b89e66b9201bd2bf929fd388fce7d8ea043c6cfba3070de64576b2eb958cc3 Severity: medium

Description: Description\ One hour before the end of a period is used to prepare for the closing of the current voting session. General users are prevented from voting to ensure that the tallying of votes can be conducted on a consistent set of data. The vote() function calls _checkEndVoteWindow()if the token is not whitelisted NFT. However, this check is missing for the reset() function, which would also affect the result.

Attack Scenario\

For tokens with a large weight, abstaining during the last hour of a period will greatly affect the results.

Attachments

1. Proof of Concept (PoC) File

https://github.com/hats-finance/Fenix--0x9d7765a7ebd5b6322a30797a44a5428531970d3d/blob/353c8e8e24454336e805e5c0e11e4e9ae1491d03/contracts/core/VoterUpgradeableV1_2.sol#L356-L363

function reset(uint256 _tokenId) external nonReentrant {
    _voteDelay(_tokenId);

    require(IVotingEscrow(_ve).isApprovedOrOwner(msg.sender, _tokenId), "!approved/Owner");
    _reset(_tokenId);
    IVotingEscrow(_ve).abstain(_tokenId);
    lastVoted[_tokenId] = _epochTimestamp() + 1;
}

2. Revised Code File (Optional)

Add the below lines before calling _reset().

IManagedNFTManager managedNFTManagerCache = IManagedNFTManager(managedNFTManager);
if (!managedNFTManagerCache.isWhitelistedNFT(_tokenId)) {
    _checkEndVoteWindow();
}

Paste the below test case after vote cant be called by nft in last hour if not whitelisted in test/nest/VoterV1_2.test.ts, and run npx hardhat test --parallel --grep "reset should not be called by nft in last hour if not whitelisted"

it('reset should not be called by nft in last hour if not whitelisted', async () => {
    let userTokenId = await deployed.votingEscrow.create_lock_for.staticCall(
      ethers.parseEther('1'),
      182 * 86400,
      signers.otherUser1.address,
    );

    await deployed.votingEscrow.create_lock_for(ethers.parseEther('1'), 182 * 86400, signers.otherUser1.address);

    await time.increaseTo((await nextEpoch()) - 3600);

    await expect(voter.connect(signers.otherUser1).reset(userTokenId)).to.be.revertedWith('distribute window');
  });

[0xmahdirostami]

OOS, Invalid

[YanhuiJessica]

@0xmahdirostami Thank you for the feedback :) > For the VoterUpgradeableV1_2.sol and VotingEscrowUpgradeableV1_2.sol contracts, in the scope includes only the changes made and their impact The check I mentioned in this issue (e.g. _checkEndVoteWindow()) was just added in this version and should not be considered OOS. oops, I just saw it mentioned in the Discord :<

[0xmahdirostami]

For now, these submissions will be judged as well.

[BohdanHrytsak]

There is a lack of data on how the user can use this thing to their advantage. And what is his benefit.

After all, he refuses to vote and receive his own reward (by shooting himself in the foot), and will not be able to vote again. Thus, he will lose the reward for his actions. And increase the reward for other users whose votes remain.

Other options are interesting, how this can be really used and where the user benefits. Since the reward of other users will not be affected, depending on the strength of the vote, the reward is calculated automatically on-chain and the last hour is not needed to calculate the reward, etc. as it is done on-chain based on votes after the era change

I agree that this can be seen as an improvement. But I would like to see a PoC of the user's benefits, and what these actions will really lead to, to make sure the severity of the problem is clear

Yes, as we can see, we also allow poke to activate the state of users in the distribution window

[YanhuiJessica]

I agree that the severity can be lowered, as the voting is for the purpose of distributing rewards. BTW, after reviewing the code again, I discovered a new issue. Although the competition is over, I'd like to let you know.

Description

VOTE_DELAY is the delay between votes in seconds. _voteDelay() checks if the user votes too frequently.

function _voteDelay(uint256 _tokenId) internal view {
    require(block.timestamp > lastVoted[_tokenId] + VOTE_DELAY, "ERR: VOTE_DELAY");
    _checkStartVoteWindow();
}

However, lastVoted[_tokenId] is always set to _epochTimestamp() + 1 after each change. In this case, if VOTE_DELAY is greater than 0, there's no voting limit after the first voting delay ends.

PoC

Paste the below test case in test/nest/Main.test.ts, and run npx hardhat test --parallel --grep "no limit after the first vote delay ends".

it(`no limit after the first vote delay ends`, async () => {
  await voter.connect(signers.deployer).setVoteDelay(86400); // 1 day

  await time.increaseTo((await nextEpoch()) + Number(await voter.distributionWindowDuration()));
  await deployed.minter.update_period();  // update _epochTimestamp()

  await voter.connect(signers.otherUser1).vote(nftToken1, [WETH_FENIX_PAIR], [1000]);
  const lastVotedAfterFirstVote = await voter.lastVoted(nftToken1);
  await expect(voter.connect(signers.otherUser1).vote(nftToken1, [WETH_FENIX_PAIR], [1000])).to.be.revertedWith('ERR: VOTE_DELAY');

  await time.increase(86400);

  expect(await voter.connect(signers.otherUser1).vote(nftToken1, [WETH_FENIX_PAIR], [1000])).to.be.not.reverted;
  const lastVotedAfterSecondVote = await voter.lastVoted(nftToken1);
  expect(lastVotedAfterSecondVote).to.be.eq(lastVotedAfterFirstVote);
  expect(await voter.connect(signers.otherUser1).vote(nftToken1, [WETH_FENIX_PAIR], [1000])).to.be.not.reverted;  // vote again without waiting for voting delays
});

[0xmahdirostami]

Lead: The original submission highlights that the reset function, which changes a vote to abstain, is not prohibited by the code, although it should be, as the general vote() function prevents this issue. This oversight could create a backdoor in the voting process, potentially allowing manipulation if large voting power is involved. This would result in the voter not voting and not receiving rewards. While the issue's potential for exploitation exists, its likelihood is low. Therefore, considering the POC and its likelihood, the issue severity is low.