0xDetermination - GaugeController: change_gauge_weight() can be frontrun to improperly increase or decrease gauge weight

[sherlock-admin2]

0xDetermination

high

GaugeController: change_gauge_weight() can be frontrun to improperly increase or decrease gauge weight

Summary

change_gauge_weight() can be easily frontrun to alter the expected result of the function call.

Vulnerability Detail

Notice how weight is set by change_gauge_weight():

def _change_gauge_weight(addr: address, weight: uint256):
    ...
    self.points_weight[addr][next_time].bias = weight
    ...

Because the weight is directly set to the desired weight, the function can be frontrun to improperly increase the gauge weight. Let's see an example:

1. Admin submits a transaction to change a gauge's weight to some value X.
2. User who has an active vote for that gauge is contributing some nonzero value Y to the gauge's bias.
3. User frontruns the admin's transaction and votes zero for the gauge.
4. After waiting for WEIGHT_VOTE_DELAY (10 days), the user votes for that gauge again, increasing the gauge's bias by Y (approximately).
5. The admin expected the gauge's weight to be X, but it is instead X + Y.

In the above example, the user is effectively granted extra voting power. It's unlikely for the admin to detect that this exploit has occurred, since no suspicious actions were taken (voting is a normal interaction).

Impact

Too many or too little rewards may be distributed. In the case of too many rewards distributed, the protocol loses funds.

Code Snippet

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

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

Tool used

Manual Review

Recommendation

Consider modifying the function to increase/decrease the weight as desired instead of directly setting it:

-def _change_gauge_weight(addr: address, weight: uint256):
+def _change_gauge_weight(addr: address, weight: int256):
    ...
-    self.points_weight[addr][next_time].bias = weight
+    self.points_weight[addr][next_time].bias += weight
    ...

[0xR3vert]

Hello,

Thanks a lot for your attention.

Thank you for your insightful observation. Upon thorough examination, we acknowledge that such an occurrence could indeed jeopardize the protocol. We are currently exploring multiple solutions to address this issue.
We are considering removing the function change_gauge_weight entirely and not distributing CVG inflation on killed gauges, similar to how Curve Protocol handles their gauges.

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

Regards,
Convergence Team

[CergyK]

Escalate

This should be of low severity
This is an admin endpoint, as such the admin is trusted to not change weights lightly, and potentially ensure that the amount of votes on a gauge is zero before calling this endpoint.

As a side note, this is an occurence of the pattern use increase/decrease instead of setting which Openzeppelin has recently abandoned on a much larger scale (their implementation of ERC20) because this was not an issue:
OpenZeppelin/openzeppelin-contracts#4585

[sherlock-admin2]

Escalate

This should be of low severity
This is an admin endpoint, as such the admin is trusted to not change weights lightly, and potentially ensure that the amount of votes on a gauge is zero before calling this endpoint.

As a side note, this is an occurence of the pattern use increase/decrease instead of setting which Openzeppelin has recently abandoned on a much larger scale (their implementation of ERC20) because this was not an issue:
OpenZeppelin/openzeppelin-contracts#4585

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.

[0xDetermination]

I don't think the reasoning given in the escalation is valid. Addressing the points made:

1. 'The admin is trusted to call the function properly'- The protocol wasn't aware of this issue before the contest, so they could have called the function improperly (note the sponsor's comment and acknowledgement). Also, admin functions aren't out of scope, only attacks requiring a malicious admin. (In this case, the issue could have been caused by a non-malicious admin.)
2. 'This issue is an occurrence of a pattern which OpenZeppelin considers to be a lower-severity issue in their ERC20 implementation'- I don't think this is relevant. The contract for which this issue was reported is not an ERC20 contract, and the impacts are completely different.

[nevillehuang]

Addressing point 2:

• In an user controlled ERC20 approval, the intention of a specific approval amount is to allow the other party to transfer that specific amount (more often than not) immediately in the first place. So you can see this almost as an user error.

• However, changing gauge weight is only callable by the admin, so In this case, it is not unreasonable to say that the admin will call change_gauge_weight() more than once to adjust weights, so this cannot be compared to an approval pattern from an ERC20, since an associated gauge weight assigned to a gauge is not expected to be of a permanent fixed value as it directly affect rewards distribution. The use case is different from an ERC20 approval.

Additionally see comments here #94 (comment)

[Czar102]

I agree that the impact for ERC20 is very different and this justification can't be used here.

I think a correct severity was selected for this issue. Planning to reject the escalation and leave the issue as is.

[Czar102]

On second thoughts, I think the current way the function works is a valid design of the protocol (though the function is a bit odd taking into account what is its main potential use case).

It should be clear that these security considerations araise and if there is a desire for the protocol to change the value by a number, they should dynamically determine the final value at the execution time, or take preventive measures to ensure the value can't be maliciously increased/decreased.

Hence, I believe the escalation made a valid point. I'm sorry for changing my position here.

[0xDetermination]

@Czar102 I think the main potential use of the function is to increase the gauge weight of certain gauges to incentivize users to stake more tokens of that gauge type.

I would agree that this classifies as 'trusted admin, therefore invalid' if the protocol was aware of this issue beforehand, but it seems that they were not.

Would like to politely request for the final judge to reconsider the escalation.

Also, this same issue was judged as M in another contest (albeit in C4: code-423n4/2023-08-verwa-findings#294)

[Czar102]

In this case I am inclined to give an example of an ERC20 approve() race condition. I will use a similar justification:

I think the main potential use of the approve function is to increase the approval to certain addresses to let them transfer from the caller.

And then any implementation of the ERC20 contract would have a medium severity issue because of the existence of the approve() function.

It is an issue if this function is intended to modify the value by an amount (which is likely when considered the context), but these are not the only use cases. The current setup makes it more difficult to operate safely, though it doesn't introduce a vulnerability if used with caution. It is a valid (though suboptimal) design choice.

The sponsor decided to fix it because they realized that they don't want to manage this complicated secure process, nevertheless we can't decide the validity per Sherlock rules on an information that hasn't been publicly available in audit resources during the time of the contest.
Hence, this issue is a good informational issue.

[nevillehuang]

@Czar102 I think you should let sponsor weigh in on this issue. Just like how you cant decide on validity when there is no publicly available information to indicate use case, the opposite is also true. Watsons have shown a possible scenario where there is a risk and protocol has acknowledged it.

I think we shouldn’t invalidate just because of the above reason, if not alot of future/current issues can be argued similarly.

[0xDetermination]

Thanks very much for the replies.

There was no published information about the specific use case of this function before the start of the contest. Therefore, no single use case can be considered as the sole intended functionality. I agree with this point made by @Czar102.

However, if no arbitrary single use case can be considered as the sole intended functionality, then every possible intended functionality must be considered as a valid use case.

Therefore, the case where the function is intended to modify the value by an amount must be considered as a valid use case, for which case this bug 'is an issue' as stated by Czar102, which I assume means it should be judged as M.

Also, I don't think the ERC20 issue is relevant here since the impacts and functionalities for that issue are completely different than this GaugeController issue.

(Additionally- this is not strictly relevant, but I can't think of any practical distinction between modifying the value by or to a certain amount.)

[Czar102]

However, if no arbitrary single use case can be considered as the sole intended functionality, then every possible intended functionality must be considered as a valid use case.

You are saying that the protocol should be doing everything it may look like it's doing. If you change the amounts by an amount in the function, you will be unable to set them to an amount without frontrunning issues, and vice versa. I hope you see where the issue with that argumentation is.

I maintain my previous judgment. Planning to accept the escalation and consider the issue informational.

[nevillehuang]

@Czar102 Could you shed more light on what you are trying to explain here? Forgive me but, I have a hard time understanding it or maybe @0xDetermination can assist if he understands it better than me.

Correct me if I’m wrong, but by your logic every function that involves admin functions should be considered invalid if there exist some form of prevention within the current code logic. So e.g #94 and #192 should be informational too since admin can take measures to prevent the issue. It is almost never possible to predict what the admins of the protocols can or cannot do unless explicitly mentioned in docs/code comments, or if its extremely obvious within the code logic.

[0xDetermination]

@Czar102 @nevillehuang I appreciate Czar pointing out the potential contradiction in my previous argument, but after thinking about it I don't think it detracts from my argument for this issue to be M. (I'm not saying that the protocol should be doing everything it may look like it's intended to do, but rather that when judging, if there was no documentation about the specific purpose of the function and it is ambiguous, since we cannot select only one potential purpose as the sole intended purpose we have to treat every potential functionality as valid for finding issues.)

If I understand it correctly, Czar's argument is that the function has two possible intended purposes/implementations that are both legitimate design choices. In this report/issue, it is being assumed that the intended purpose of the function is changing the gauge weight by a certain amount (incrementing/decrementing), in which case this issue would be a valid M. But, as Czar pointed out, another possible purpose of the function is strictly changing the gauge weight directly to a certain value, in which case this report would be invalid.

But the problem here is that we have already assumed that the exact intended purpose of the function is unknown. So we still have to equally consider both cases. In the to case, I don't think there will be any loss of funds with frontrunning, so any frontrunning issue would be invalid/low. In the by case, there can be loss of funds with frontrunning. So it seems to me that the issue should still be considered M.

I also don't think the issue should be downgraded because as Czar stated, the most likely use case of the function is changing the weight by an amount. I really don't think the other case is likely to be the intended functionality. Furthermore, a valid attack path resulting in loss of funds was presented in this issue. The sponsor also acknowledged this issue (although maybe this is not relevant, as Czar stated earlier).

I really don't think it's right to downgrade this issue, both for the technical reasons given and for the 'soft' reasons given in the above paragraph.

Thanks very much to both judges for reading all my comments.

[CergyK]

After rereading the issue:

After waiting for WEIGHT_VOTE_DELAY (10 days), the user votes for that gauge again, increasing the gauge's bias by Y (approximately).

For 10 days the gauge_weight has the weight the admin wanted by calling the function?

Also the admin is not disabling the gauge by changing the weight, so he can reasonably expect that some legitimate users will be able to vote for it and increase the value?

Sounds like a non-issue

[nevillehuang]

@CergyK I think this similar finding in C4 along with explanations and PoC in the comments provide a good context as to why this could be an issue. The fact that the protocol plans to remove this functions signifies a risk that they are not willing to take.

[0xDetermination]

@CergyK For 10 days the weight is as the admin desired, but after that 10 days the weight can be higher than the admin desired. The weight can remain higher for the length of the frontrunning user's lock time.

[nevillehuang]

@0xDetermination What you are mentioning are infact the same scenarios, thats why I'm confused as to what head of judging @Czar102 is pointing to. If you want to change the gauge directly to a certain value than it will involve a increment/decrement.

If I understand it correctly, Czar's argument is that the function has two possible intended purposes/implementations that are both legitimate design choices. In this report/issue, it is being assumed that the intended purpose of the function is changing the gauge weight by a certain amount (incrementing/decrementing), in which case this issue would be a valid M. But, as Czar pointed out, another possible purpose of the function is strictly changing the gauge weight directly to a certain value, in which case this report would be invalid.

[CergyK]

@CergyK I think this similar finding in C4 along with explanations and PoC in the comments provide a good context as to why this could be an issue. The fact that the protocol plans to remove this functions signifies a risk that they are not willing to take.

The original issue did not have the 10 days delay, so the gauge weight could end up having a different value than what intended by the admin in the same block.

Here the "attacker" changes the value 10 days after that, sounds pretty much like harmless legitimate usage

[nevillehuang]

@CergyK Pretty sure this is still not the intention of the protocol to allow influence of gauge weight. It may "sounds like legitimate use" to you but not to others, since we don't now how long a voting period lasts.

[0xDetermination]

@nevillehuang in the to scenario it would be setting directly with '=' rather than increment/decrement, but like I mentioned I don't think this is likely to be the intended function. I also doubt that the dev who wrote this was thinking specifically about to or by as we are discussing it.

[Czar102]

I believe I posted my explanations, though judgments like this can never be made fully objectively, since there is no objective set of rules.

But the problem here is that we have already assumed that the exact intended purpose of the function is unknown. So we still have to equally consider both cases.

That would mean that the lack of the documentation itself will cause a valid issue to be found, no matter what the implementation is. Hope you see my point.

I agree that the increment/decrement would be the most likely use case, but then we need to consider the ability for admins to derive the "set" value at runtime, hence mitigating possible discrepancies of results in these two representations of actions. This is my main point in these considerations.

I must say I appreciate all the comments, and it's awesome to see @0xDetermination understanding all points of view.

[nevillehuang]

@Czar102 Till now I don't quite understand the basis of invalidating this issue. But what I can say is, I respect your decision and this is fair enough, given this is what we signed up for when competing in this contests.

I think sherlock has alot to work on for contest details and information, which I have advocated quite a few times already and even given explicit suggestions. I hope that will be taken into consideration so this type of "documentation" error can be replaced by factual issues with factual evidences and statements, instead of coming in with subjective comments from all parties.

[Czar102]

I agree, we have to work on that.

[Czar102]

Result:
Low
Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• CergyK: accepted

[IAm0x52]

Fix looks good. _change_gauge_weight has been removed