bughuntoor - Killing a gauge will result in a user's votes being unusable for up to 10 days

[sherlock-admin2]

bughuntoor

medium

Killing a gauge will result in a user's votes being unusable for up to 10 days

Summary

A user may have his votes unusable for up to 10 days, upon a gauge's removal

Vulnerability Detail

Within the GaugeController, users vote for the gauge of their choice. They allocate a power of their voting power which is based on their voting escrow.

@internal
def vote_for_gauge_weights(tokenId: uint256, _gauge_addr: address, _user_weight: uint256):
    """
    @notice Assign/update voting power to a gauge to add to its weight, and drag more/less inflation onto it.
    @dev For a killed gauges on.
    @param _gauge_addr Gauge which `msg.sender` votes for
    @param _user_weight Weight for a gauge in bps (units of 0.01%). Minimal is 0.01%. Ignored if 0
    """
    assert not self.isLock, "VOTE_LOCKED"
    assert not self.killed_gauges[_gauge_addr] or _user_weight == 0 , "KILLED_GAUGE"
    assert self.vote_activated[_gauge_addr], "VOTE_GAUGE_PAUSED"

    lockingManager: LockingPositionManager = self.control_tower.lockingPositionManager()
    lockingDelegate: LockingPositionDelegate = self.control_tower.lockingPositionDelegate()

    # Check if the sender is the owner or a delegatee for the token.
    assert (lockingManager.ownerOf(tokenId) == msg.sender or lockingDelegate.delegatedVeCvg(tokenId) == msg.sender), "TOKEN_NOT_OWNED"
    # Check whether the token is time-locked: a token can be time-locked by its owner to protect a potential buyer from a malicious front run.
    assert (lockingManager.unlockingTimestampPerToken(tokenId) < block.timestamp), "TOKEN_TIMELOCKED"
    escrow: VotingPowerEscrow = self.control_tower.votingPowerEscrow()
    slope: uint256 = convert(escrow.get_last_nft_slope(tokenId), uint256)
    lock_end: uint256 = escrow.locked__end(tokenId)
    _n_gauges: int128 = self.n_gauges
    next_time: uint256 = (block.timestamp + WEEK) / WEEK * WEEK
    assert lock_end > next_time, "Your token lock expires too soon"
    assert (_user_weight >= 0) and (_user_weight <= 10000), "You used all your voting power"
    assert block.timestamp >= self.last_nft_vote[tokenId][_gauge_addr] + WEIGHT_VOTE_DELAY, "Cannot vote so often"  // @audit - important line 

In order to not spam-change their votes, users have a WEIGHT_VOTE_DELAY of 10 days. Meaning they cannot change their vote towards a gauge for 10 days.

However, there's an admin function, kill_gauge, allowing the admins to kill a gauge. Upon killing a gauge users can remove their votes from said gauge. However, in order to do so, they still have to go through the 10-day timelock, thus meaning their votes are locked and unused/unusable for up to 10 days.

    assert block.timestamp >= self.last_nft_vote[tokenId][_gauge_addr] + WEIGHT_VOTE_DELAY, "Cannot vote so often"

Impact

Users voting power may be unusable for up to 10 days

Code Snippet

https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/Locking/GaugeController.vy#L640

Tool used

Manual Review

Recommendation

Change the check on #L640 to the following

    assert self.killed_gauges[_gauge_addr] or block.timestamp >= self.last_nft_vote[tokenId][_gauge_addr] + WEIGHT_VOTE_DELAY, "Cannot vote so often"

[0xR3vert]

Hello,

Thanks a lot for your attention.

Indeed, when a gauge is killed, any user who voted on it will be unable to retract their vote if their last vote was cast within the past 10 days.
So it will be fixed even if it's not viewed as a significant concern since it does not result in a permanent blockage of user funds.

Therefore, in conclusion, we must consider your issue as a valid.

Regards,
Convergence Team

[nevillehuang]

This has the same root cause as #192, but the impact described is different. I think it still warrants medium severity given voting on proposals are time-sensitive and can have a impact on deciding whether certain proposals are executed or not. I'm not sure if duplicating with #192 is the right thing to do given the impact have different severity levels, but I am leaning towards yes.

[nevillehuang]

Have consulted @Czar102, and agree that #58 and #218 should be duplicates of #192, given it is clear that if impacts result from a single root cause, then they should be duplicated based on sherlock duplication rules.

While not the best way of handling things given the impact is vastly different, I believe this is the appropriate decision to make as of now.

[deadrosesxyz]

Escalate
Disagree with judge's decision. #58 and #218 should not be marked as dups. The root problem of #58 and #218 is the vote delay mechanism. The root for #192 is incorrect handling of bias/ slope when calling change_gauge_weight

[sherlock-admin2]

Escalate
Disagree with judge's decision. #58 and #218 should not be marked as dups. The root problem of #58 and #218 is the vote delay mechanism. The root for #192 is incorrect handling of bias/ slope when calling change_gauge_weight

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[nevillehuang]

@deadrosesxyz I agree with you, thats why I initially separated them. Unfortunately, the root cause of killing gauges is the same albeit the impact is vastly different, and so @Czar102 has advised me to keep them as duplicates based on current rules and I have his word that sherlock will have better rules in place to separate such issues in the future.

[deadrosesxyz]

@Czar102 Could you please kindly elaborate on this? Root cause, impact and fix seem all to be different.

[Czar102]

The escalation states:

#58 and #218 should not be marked as dups. The root problem of #58 and #218 is the vote delay mechanism.

I think it was meant that #58 and #192 aren't duplicates?

[deadrosesxyz]

I may have worded the escalation poorly.

#58 and #218 are duplicates of each other, but I believe they should not be marked as duplicates of #192. Instead, #58 and #218 should be marked as a separate issue.

[Czar102]

I see. I also can't notice why #192 would be a duplicate, to me it looks like the issues are triggered at the same time, but they have different root causes and different impacts.
@nevillehuang may I ask you to elaborate why do you think the root causes are the same? The root cause is a fragment of code that can be corrected to remove the issue.

[nevillehuang]

@Czar102 Since the fix involve would be potentially different, I can agree that they are not duplicates. I believe delaying voting is a medium severity issue since it can be seen as a DoS with no direct loss of funds.

[Czar102]

This seems like a design decision to me, I don't see this exceeding info/low severity. @nevillehuang what do you think?

[nevillehuang]

@Czar102 the protocol has confirmed this is not a design decision, it is not intended to forcefully lock user from voting for 10 days especially when its time sensitive. This constitutes medium severity for me.

[Czar102]

This lock for up to 10 days is as if the user decided to unstake. Now, if the unstake function allowed an admin to be used as the user, that would be expected behavior and the admin should just take this into account if they are unstaking for the user. Here, the admin unstakes like that for all users.

In general, the admin can make an announcement not to stake in a pool 10 days before killing a gauge. This way, the function is properly used, and there is no lock.

The fact that the protocol team intends to apply the proposed improvement is not a signal of the severity of the issue.

[nevillehuang]

@Czar102 Same thought as in this issue comment. Its a bit unreasonable how you have to come in and subjectively explain the supposedly correct intention of the protocol when it clearly deviates from protocol intentions mentioned by the sponsor. Whatever it is, fair game, it is what we signed up for.

I implore you as head of judging, me as lead judge and all watsons to work towards providing actual factual comments in the future, with the help of possibly improved contest documentation and resources.

[Czar102]

Its a bit unreasonable how you have to come in and subjectively explain the supposedly correct intention of the protocol when it clearly deviates from protocol intentions mentioned by the sponsor.

I am not commenting what the intention of the protocol was, but whether the design of the code is a plausable one. If it is a plausable design choice, then it's clearly not a vulnerability.

[Czar102]

Result:
Invalid
Unique

(duplication status doesn't matter)

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• deadrosesxyz: accepted

[nevillehuang]

@Czar102 You do you, but I am confused about your stance, you have the final say, and I will respect it.

[IAm0x52]

Fix looks good. Check fixed when killing gauge