GuildToken transfers are not allowed if any of the initial gauges of the user has a loss that has not been applied to the user

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20Gauges.sol#L500

Vulnerability details

Impact

In GuildToken.sol a comment says "Even when a loss occur, users can still transfer tokens with which they vote for gauges that did not suffer a loss" which is perfectly logical thing to expect. Other additional comments here mean that the user should be able to transfer Guild tokens which have not been used to add weight to a loss-suffering gauge.

But this is not true for one case.

The GuildToken.transfer function calls ERC20Gauges._decrementWeightUntilFree function to free up gauge weights from all his gauges (starting from the first entry in the userGauges list) until sufficient weight has been freed such that the desired guild tokens can be transferred (provided more than the desired amount are actually existing as free from any gauge, or put in non-slashed gauges so that they can be withdrawn, or free from debtCeiling constraints : essentially freeweights).

Suppose user has 1000 GUILD tokens and he allocates them to 10 different gauges A, B, C..... ... these gauges are added to the userGauges list sequentially as the user incrementGauge for them and exist in that particular order until user decides to remove entire weight from one of them( or gets losses to a gauge) and that gauge is then removed from the list.

So right now, A, B, C all have 100 weight from the user and there are zero freeWeights. Now assume that gauge B suffers a loss so that 100 guild token will be locked. Now other 900 guild token should be accessible to transfer etc. because they have not suffered any losses, so user should be free to free/reallocate/transfer them.

But if user wants to transfer 200 Guild tokens (basically anything > 100), then it will fail. This causes revert on the entire transfer txn and most of the user's guild tokens may get locked for arbitrary time

This is because in the transfer, _decrementWeightUntilFree is called which starts freeing gauge weights from the user using _decrementGaugeWeight for each gauge starting from the first gauge in the user list (which have been put into the list sequentially as the user entered those gauges). In i = 0 , 100 gauge weights gets freed, so totalFreed = 100. Now in i = 1, _decrementGaugeWeight tries to free weights from gauge B (where a loss has been notified but loss has not been applied to) which will fail due to the require statement here

But user still has 900 total guild tokens which he should be able to transfer/free but the intermediate gauges in the list that might have suffered losses cause transfers to fail. So even though the user has the capacity to transfer tokens, he will not be able to, so all his other 800 tokens have also become non-transferable (apart from if he only tries to free 100 in the given example, but the given example has so many different possible variations too, like if the first gauge A suffers a loss, this will make all 900 tokens as non-transferrable)

Guild token transfers will be blocked when they shouldn't be.

Proof of Concept

_decrementWeightUntilFree function tries to free weights starting from the first gauge acc to userGauges list recorded entries :
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20Gauges.sol#L520

_decrementGaugeWeight will fail and cause revert for the entire transfer txn if a loss that has happened has not been applied to the user for that gauge yet
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L214

This revert is also possible due to another reason : if debtCeilings have been utilized for one of the initial gauges. I have described this impact in detail in another finding because it has a separate root cause and separate fix.

Tools Used

Manual review

Recommended Mitigation Steps

Refactor the _decrementWeightUntilFree loop to continue; if the loop reaches a gauge that has an unapplied loss for the user, skip this iteration and continue execution with the next gauge in the userGauges list. This way, the user can free all weights that should be transferrable by design.

Assessed type

Context

[c4-pre-sort]

0xSorryNotSorry marked the issue as insufficient quality report

[0xSorryNotSorry]

Same submission here: #1107

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #152

[c4-judge]

Trumpero marked the issue as unsatisfactory:
Insufficient quality

[c4-judge]

Trumpero marked the issue as not a duplicate