0xDetermination - LockingPositionDelegate: Addresses can be gas griefed and permanently DoSed such that they are unable to receive delegations

[sherlock-admin]

0xDetermination

medium

LockingPositionDelegate: Addresses can be gas griefed and permanently DoSed such that they are unable to receive delegations

Summary

Any address can be permanently DoSed such that it cannot receive delegations* (veCVG and/or mgCVG). Furthermore, gas griefing is possible.

*(technically, it still receives spam delegations from the attacker)

Vulnerability Detail

The explanation here will omit the case for mgCVG, as it's more or less the same as the case for veCVG.

Let's first consider the gas griefing impact. Consider the following code:

    function delegateVeCvg(uint256 _tokenId, address _to) external onlyTokenOwner(_tokenId) {
        require(veCvgDelegatees[_to].length < maxTokenIdsDelegated, "TOO_MUCH_VE_TOKEN_ID_DELEGATED");
        ...
        if (_to != address(0)) {
            /** @dev Add delegation to the new address.*/
            veCvgDelegatees[_to].push(_tokenId);
        }

As can be seen, every address has a limit to how many delegations it can have. (This limit is set to 25 in the initializer.) Also note that LockingPositionService.mintPosition() does not have a minimum mint amount, so the cost in CVG to the attacker will be negligible. The attack is simple:

1. Attacker mints 24 tokens and delegates them to the victim address.
2. The address cannot receive any more delegations, since the limit has been reached.
3. To enable the address to receive more delegations, it needs to call removeTokenIdDelegated() and remove at least one token from its delegator list. Note that delegators can only be removed one at a time.

The gas cost to remove a delegator from a full list is at least 24 cold SLOADs (2100 gas each) due to the below code that's run when removing a delegator:

    function getIndexForVeDelegatee(address _delegatee, uint256 _tokenId) public view returns (uint256) {
        uint256[] memory _tokenIds = veCvgDelegatees[_delegatee];
        ...

Therefore the gas cost is at least $24 * 2100 + 21000 = 71400$, where 21000 is the base EVM transaction cost. The cost to the attacker for each delegating transaction (ignoring minting costs) is around 3 cold SLOADs (2100 gas each), 1 cold SSTORE (22100 for zero to nonzero), and 1 warm SSTORE (20000 for zero to nonzero), plus the base transaction cost. Thus the cost to the attacker to delegate one token is around 70,000 gas. It can be said that the cost/damage ratio is about 1:1.

Now, let's consider the permanent DoS attack. There are two possible attack scenarios. One attack uses frontrunning:

1. The attacker delegates 24 tokens to the victim address.
2. The victim address wants to be able to receive delegations, so it calls removeTokenIdDelegated() for one of the attacker's tokens.
3. The attacker frontruns the transaction with two attacking transactions. The first undelegates the token by calling delegateVeCVG() for address(0), and the second delegates a new token to the victim.
4. The victim's transaction will fail, since it is attempting to undelegate a token that is not currently delegated to the victim. The result is that the victim is still unable to receive delegations.

The other attack uses backrunning and is cheaper, but could potentially fail:

1. The attacker delegates 24 tokens to the victim address.
2. The victim address wants to be able to receive delegations, so it calls removeTokenIdDelegated() for one of the attacker's tokens.
3. The attacker simply backruns the victim's transaction and redelegates the token (or delegates another token) to the victim. The result is that the victim is still unable to receive delegations.

The gas costs to the victim and the attacker in the backrunning scenario are about the same as in the gas griefing scenario; that is, 1:1.

In the frontrunning scenario, the attack is more expensive. The victim's transaction only uses 1 cold SLOAD before reverting, so the cost to the victim is around 25000 gas. In comparison, the attacker's first transaction executes 3 cold SLOADs and 1 warm SSTORE (2900 since the value is not changed to nonzero from zero), so the cost is around 30000 gas. We can assume that the second transaction costs around 70000 gas as previously explained, so the total cost to the attacker is around 100,000 and the ratio of the attacker cost to the victim cost for the guaranteed DoS attack is around 4:1.

Impact

Any address can be made permanently incapable of receiving delegations (DoS). If a victim address wants to receive delegations, it will need to spend gas to enable itself to do so (gas griefing, cost/damage ratio around 1:1).

Code Snippet

https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Locking/LockingPositionDelegate.sol#L249

https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Locking/LockingPositionDelegate.sol#L285

https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Locking/LockingPositionService.sol#L238

https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Locking/LockingPositionDelegate.sol#L66

https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Locking/LockingPositionDelegate.sol#L248-L322

https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Locking/LockingPositionDelegate.sol#L376-L383

https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Locking/LockingPositionDelegate.sol#L422-L439

https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Locking/LockingPositionDelegate.sol#L466-L483

Tool used

Manual Review

Recommendation

A whitelist could be implemented to prevent malicious actors from spam delegating.

If a whitelist is undesirable, the code architecture may require significant changes to fix this issue. If possible, keeping track of delegatee state should be done without storing lists of delegators.

[0xDetermination]

Escalate

This issue is marked as a dup of #142, which I believe does not show sufficient impact. In my submission here, I demonstrate a gas griefing impact and a permanent DoS impact.

The judge marked issue 142 as invalid with the reasoning 'Agree with sponsor, since cleanDelegatees() and removeTokenIdDelegated() here allow removal of delegatees'. This is true for issue 142, but it can be seen that neither of these two functions can be used to prevent the permanent DoS and gas griefing attacks described in my submission here.

[sherlock-admin2]

Escalate

This issue is marked as a dup of #142, which I believe does not show sufficient impact. In my submission here, I demonstrate a gas griefing impact and a permanent DoS impact.

The judge marked issue 142 as invalid with the reasoning 'Agree with sponsor, since cleanDelegatees() and removeTokenIdDelegated() here allow removal of delegatees'. This is true for issue 142, but it can be seen that neither of these two functions can be used to prevent the permanent DoS and gas griefing attacks described in my submission here.

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]

Would like to add- I believe the duplicates of this issue #51, #49, #206, #212, and #202 do not demonstrate sufficient impact to be judged as H/M.

Also would like to mention that #142 is specifically for mgCVG delegation whereas this issue is primarily for veCVG delegation.

[nevillehuang]

@0xDetermination

This has the exact same root cause of the maxTokenIdsDelegated check as all the above mention issues, albeit not in the same function and potentially the same fix, so suggestion is to keep as duplicate.

I will have to think more about the duplicates, given almost all have mentioned the correct root cause of abusing the max delegated cap.

142, #51, #49, #165 - I think this are definite duplicates, some forms of front running/back running mentioned.

212 - Seems to imply front-running, but debatable

206, #202 - Likely invalid, attack path described not sufficient.

[0xDetermination]

@nevillehuang Thank you very much for the detailed analysis. I took a look at these issues again and I agree with you on most of them. Have a few things I would like to mention, please let me know if my reasoning is good:

51 only describes the backrunning attack which is not 100% reliable- note that the potential failure of the backrunning attack is mentioned in this issue 165. (A backrunning attack can be circumvented with a flashbots bundle, or the backrunning tx simply might not be executed before another delegation tx). Similarly, #49 was submitted by the same watson, and it also does not describe a guaranteed DoS of over 1 year. I'm not sure if these two issues should be considered valid Medium/dup or valid low in this case, since DoS of under 1 year seems to be generally considered as low.

212 doesn't seem to imply frontrunning, and the recommendation is somewhat unclear and suggests that the watson was not aware of the cleaning/removal functions. I don't think this should be considered a Medium.

Will respect your final decision.

[ChechetkinVV]

@0xDetermination

This has the exact same root cause of the maxTokenIdsDelegated check as all the above mention issues, albeit not in the same function and potentially the same fix, so suggestion is to keep as duplicate.

I will have to think more about the duplicates, given almost all have mentioned the correct root cause of abusing the max delegated cap.

142, #51, #49, #165 - I think this are definite duplicates, some forms of front running/back running mentioned.

212 - Seems to imply front-running, but debatable

206, #202 - Likely invalid, attack path described not sufficient.

I continue to insist that the above reports describe different problems. Using a whitelist will not solve the problem for the token owner when one delegate needs to be de-delegated, although the intendent behavior of the delegateVeCvg function is different.

There is no need for a special attack here. The function breaks down during normal user actions. This is exactly what is stated in Impact #206. So the owner of the token will delegate to trusted persons, he will be confident in their honesty and the absence of fraudulent activities, but as a result he will face the same problem. This will apply to all users without exception.

I can’t comment on the validity of other items, because I don’t understand the benefit of an attacker in DOS of individual user addresses. And many questions arise: How does an attacker select addresses to attack? What will an attacker do if the delegate can accept delegation to different addresses?

[0xDetermination]

@ChechetkinVV I see your point, the different impact in #206 is that users will be unable to un-delegate once max delegations are reached (unless the delegatee cleans the list of delegated tokens). I agree that a whitelist would not fix the issue for this case. I also think you could make an argument for issue #206 to be considered separate from the rest of the delegation issues, but I don't think this specific impact can be considered H/M since it does not detail loss of funds or a DoS of over 1 year.

Regarding benefit to the attacker- this is a griefing issue, and as a result there is no (obvious) benefit to the attacker.

I think #142 implies a permanent DoS impact, so I won't argue for that issue to be judged low/invalid.

I also don't think any of the other submissions demonstrate sufficient impact to be judged as H/M, as none of them describe DoS of over 1 year or gas griefing.

[nevillehuang]

I agree with @0xDetermination, the only attack vector here sufficient to make medium severity via a >1year or perhaps even permanent DoS is the possibility of front-running/backrunning a delegation removal from a delegator to invoke the max delegation check . If this isn't explicitly mentioned, seems to be hard to justify the severity on grounds of unclear attack path.

[Czar102]

I think griefing is not sufficient of an impact to be considered H/M according to Sherlock rules. Please note that frontrunning to get the transaction to fail is not a 1 year DoS, but a 1-2 block DoS. Doing that multiple times doesn't forbid the griefed user to bundle the transactions they want to make. The protocol will be on mainnet, so ways to bundle transactions exist (Flashbots, for example). Nevertheless, would be good to see the protocol team fixing this. Tagging @0xR3vert @shalbe-cvg @walk-on-me.

Planning to leave the issue as is.

[nevillehuang]

@Czar102 , I didn't know bundling transactions using flashbots can be taken into considerations for such issues, will keep that in mind in the future. Best to make it clear to watsons in maybe sherlock rules, if not it can cause alot of unnecessary discussions in the future.

[0xDetermination]

@Czar102 @nevillehuang Thanks for the reply Czar102; I don't think tx bundling can fix this issue since the attacker can cause any removeTokenIdDelegated() call from the victim to revert. The attacker can just use a frontrunning bot to cause permanent DoS. I think this should be a Medium unless the cost/damage ratio of the guaranteed DoS vector is considered as too high.

[Czar102]

The attacker can just use a frontrunning bot to cause permanent DoS.

I don't see how frontrunning causes a permanent DoS here. Please make sure you read my previous comment.

[0xDetermination]

@Czar102 Thanks very much for the reply. If I understand correctly, your point is that one frontrun will cause DoS of only one undelegating transaction, so the DoS impact is only 1 block and therefore does not constitute DoS of over 1 year.

My argument is that the attacker can use a frontrunning bot to cause any undelegating transaction to fail, effectively causing permanent DoS. Example:

1. Attacker spam delegates tokens to the victim's address until the limit is reached.
2. Victim submits a transaction or bundle to undelegate any number of the attacker's spam delegations.
3. Attacker frontruns (with a bot) by undelegating the targeted spam delegations, and delegating with other spam tokens that were not targeted by the victim.
4. The victim's undelegating transactions fail, because the token IDs targeted are not delegated to the victim's address anymore due to the attacker's frontrunning transactions. Since the number of tokens delegated to the victim address is still at the max limit, the victim cannot receive any new delegations.
5. Repeat steps 2-4, resulting in permanent DoS.

I think most of the marked duplicates of this issue only mentioned the backrunning attack vector, or otherwise failed to mention the guaranteed DoS vector via frontrunning; in these cases, transaction bundling would indeed circumvent the attack.

Please kindly let me know if I am missing any key detail here.

[Czar102]

I understand the attack path, my point was that repetitively DoSing for a single transaction (griefing) is not considered a DoS for more than one year. Someone can simply use a private mempool to get their transaction accepted at any point during that time.

Also, frontrunning doesn't change the crux of the attack – at this first (or second) stage, it is not important who undelegates the tokens, only provides a slightly more comfortable way of adding new tokens by the attacker. Hence, I don't think this observation provides a strong enough attack path to change the severity.

[0xDetermination]

@Czar102 Thanks very much for the reply, apologies for the invalid argument.

[Czar102]

Result: Low Unique

(duplication status doesn't matter)

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xDetermination: rejected