Zero-Amount Vulnerability in migrate Function Allows Unauthorized Migration

[hats-bug-reporter]

Github username: @0xmahdirostami
Twitter username: 0xmahdirostami
Submission hash (on-chain): 0x2ff4aa7233cf3b4cb007e0c3798cbe9f9d27479b385ace46376936fa23ada56c
Severity: low

Description:

Description

The current implementation of the migrate function lacks a check to ensure that the amount being migrated is greater than zero. This omission allows malicious users to exploit the function by migrating other users to V2, even with a zero amount.

ITokenV1 circlesV1 = ITokenV1(hubV1.userToToken(_avatars[i]));
if (address(circlesV1) == address(0)) {
    // Invalid avatar, not registered in hub V1.
    revert CirclesAddressCannotBeZero(2);
}
convertedAmounts[i] = convertFromV1ToDemurrage(_amounts[i]);
// transfer the v1 Circles to this contract to be locked
circlesV1.transferFrom(msg.sender, address(this), _amounts[i]);

The migration process must only allows users to migrate balances they own themselves but the function does not validate _amounts[i] to be greater than zero, an attacker could repeatedly execute the function with a zero amount.

Impact

migration function not working as intended. users could migrate other users even without holding their token.
it would be even worse if those users want to register as org or group which will not be applicable anymore

Mitigation

Add a check to ensure that the migration amount is greater than zero, preventing zero-amount migrations:

        require(_amounts[i] > 0, "Amount must be greater than zero");

[benjaminbollen]

leaving provisional comment here. This issue is correct in its assessment and pinpoints the issue correctly (and made me realize the deviation between intended behaviour and actual code behaviour)

there is some possible contention with #4. While the title of #4 makes (in the end) the "right claim" it did it with wrong arguments and I declined it for those reasons - at the time I disagreed with the claimed title.

However this issue actually highlights the actual reason why "unauthorized migrations are possible"

I will note that I don't see this as an attack or bad behaviour; if some "attacker" wants to spend gas to forcefully help register the v1 graph in v2", I would try to find out where they live so I can send a thank you card and a box of chocolates. With a finer point:

it would be even worse if those users want to register as org or group which will not be applicable anymore

this is not correct though; in v1 only humans can mint a token (groups are not part of v1); so only v1 humans can be auto-registered as humans in v2. Hence the box of chocolates for the attacker paying for the migration cost out of his own pocket.

That said, it is a deviation from the intention that was (with common sense) behind this function, so I think this issue #50 is a fair 'low' issue

[aktech297]

leaving provisional comment here. This issue is correct in its assessment and pinpoints the issue correctly (and made me realize the deviation between intended behaviour and actual code behaviour)

there is some possible contention with #4. While the title of #4 makes (in the end) the "right claim" it did it with wrong arguments and I declined it for those reasons - at the time I disagreed with the claimed title.

However this issue actually highlights the actual reason why "unauthorized migrations are possible"

I will note that I don't see this as an attack or bad behaviour; if some "attacker" wants to spend gas to forcefully help register the v1 graph in v2", I would try to find out where they live so I can send a thank you card and a box of chocolates. With a finer point:

it would be even worse if those users want to register as org or group which will not be applicable anymore

this is not correct though; in v1 only humans can mint a token (groups are not part of v1); so only v1 humans can be auto-registered as humans in v2. Hence the box of chocolates for the attacker paying for the migration cost out of his own pocket.

That said, it is a deviation from the intention that was (with common sense) behind this function, so I think this issue #50 is a fair 'low' issue

Hey... I think the issue #4 already pointed the issue. #50 is duplicate of the #4

[bronzepickaxe]

Hi @benjaminbollen,

A correct submission title is not a prerequisite of a valid finding. In other words, if the body of finding A describes a valid issue but it uses title B that is invalid, it still means that finding A is valid.

Furthermore, the inverse of this logic has been applied to validate this finding. You rightfully mention that:

• it would be even worse if those users want to register as org or group which will not be applicable anymore

is not correct statement made in this report.

If we were to apply the same logic that has been used to invalidate #4, being:

• wrong title/argument + right conclusion = invalid finding

than this finding should be invalid as well according to that logic, since one of the premises is wrong.

However, we can't fixate on every single line in a report and invalidate a whole report if a single line is wrong. Thus, both these findings are valid, however, this finding is a duplicate of #4.

[benjaminbollen]

Im not opening this discussion. It is very clear - and I have stated - that issue #4 has "the right title", but "none of the valid arguments".

In other words, if the body of finding A describes a valid issue but it uses title B that is invalid, it still means that finding A is valid.

So @bronzepickaxe turning this upside down is not a fair characterization to even continue a discussion on. There is no discussion here.

[benjaminbollen]

Just re-confirming I have read issue 4 and issue 50 and the decision is very clear cut: the valid low issue concerns that there is no check on the amount being not-zero, which is the point raised in this issue 50, and issue 4 discussion has several wrong arguments, but never mentions to one critical point: a check on non-zero amount, instead falsely argues neglecting the revert caused by the transfer call.

so I hold, 50 is a valid low issue and not a duplicate, and 4 was invalid