sherlock-audit / 2023-09-Gitcoin-judging

11 stars 7 forks source link

0xHelium - Malicious user can spam create profiles for other users #7

Closed sherlock-admin2 closed 11 months ago

sherlock-admin2 commented 11 months ago

0xHelium

medium

Malicious user can spam create profiles for other users

The Registry.createProfile() allow any person to create profile for anyone else without any limitation.This open door for spammer to show off their spam artist talents.

Vulnerability Detail

The Registry.createProfile() allow any person to create profile for anyone else without any limitation. Because of the way this protocol intend to be used , the person who the malicious user created the profile for can use that profile just by having the Id whiich was created from the keccak256 hash of the malicious user account and a random nonce; Although the the person who the malicious user created the profile for is not directly exposed to a loss of funds, this is still an issue as a spammer can create up to type(uint256).max number of profiles for the same victim account.

Impact

Copy and paste this function into Registry.t.sol

function test_MaliciousUsersCreateProfileForOthers(
        string memory _name,
        address malicious_user,
        address owner1
    ) public {
        vm.assume(malicious_user != address(0));
        vm.assume(owner1 != address(0));
        vm.assume(owner2 != address(0));

        vm.startPrank(malicious_user);
        uint256 _nonce = 123;
        uint256 _nonce2 = 1234;
        bytes32 profileId_1 = registry().createProfile(_nonce, _name, metadata, owner1, profile1_members());
        bytes32 profileId_2 = registry().createProfile(_nonce2, _name, metadata, owner1, profile1_members());

        Registry.Profile memory profile1 = registry().getProfileById(profileId_1);
        Registry.Profile memory profile2 = registry().getProfileById(profileId_2);

        assertEq(profile1.id, profileId_1);
        assertEq(profile2.id, profileId_2);

    }

and run the test, it passes , so a spammer can create as many profile as he wants for his victims, and if by any mean he can trick the victims into using the profile Id, there is a possibility that he scams the victims.

Code Snippet

https://github.com/sherlock-audit/2023-09-Gitcoin/blob/main/allo-v2/contracts/core/Registry.sol#L119-L168

Tool used

Manual Review

Recommendation

I recommend ensuring only the caller can create a profile for owner param. This can be done by reverting the function if owner param is not msg.sender.

sherlock-admin commented 11 months ago

1 comment(s) were left on this issue during the judging contest.

n33k commented:

invalid, expected behavior

neeksec commented 11 months ago

This is a low severity finding.

spammer can create up to type(uint256).max number of profiles for the same victim account

Creating up to type(uint256).max number of profiles costs too much gas that no one can afford.

For owner to use the malicious profile, an extra spam step is required. So I would categorize it as low severity.

quentin-abei commented 11 months ago

Escalate

sherlock-admin2 commented 11 months ago

Escalate

  • This is not an expected behavior as @neeksec understood at first because sponsor confirmed it.
  • type(uint256).max is just the theorical number of spam profile one can create, it doesn't mean that one have to create all that in order to setup his scam, he can well only create one or two.So the gas cost argument is irrelevent.
  • Since this is not expected behavior and sponsor will fix it means that it exposes a function malfunction i.e function does not work as intended, and when function does not work as intended it's automatically a valid medium.
  • Because of all this: this issue is well a valid medium.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

thelostone-mc commented 11 months ago

Would call this out as a medium severity @quentin-abei

neeksec commented 11 months ago

Escalate

on behalf of sponsor and watson. Since sponsor agree with a medium.

sherlock-admin2 commented 11 months ago

Escalate

on behalf of sponsor and watson. Since sponsor agree with a medium.

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 commented 11 months ago

Would call this out as a medium severity @quentin-abei

This statement below says if creator can trick the victims he can scam them, but it has no relation to any issue within the protocol at all. Unless the watson can prove a explicit loss of funds/breaking of core functionality due to this issue, I am finding it hard to see the medium severity for this issue, because the necessary access control is in place for profiles and the creator will just spend a ton of unnecessary gas. Could you care to elaborate @thelostone-mc ?

so a spammer can create as many profile as he wants for his victims, and if by any mean he can trick the victims into using the profile Id, there is a possibility that he scams the victims.

quentin-abei commented 11 months ago

Let me explain how a malicious user can cause a loss of funds with this exploit: For the needs of this demo i will call malicious user userA.

Because of the above steps, i consider my issue at least a valid medium. @thelostone-mc , @neeksec

nevillehuang commented 11 months ago

Escalate

I still don’t see how its Medium severity. It is users responsibility to verify that they are funding the correct pool. Even with the fix implemented by sponsor, if malicious profile creators intend to scam users, they can still market themselves as ‘vitaliks’ address. IMO this falls under user input error and my discussion here is done, will let @hrishibhat decide.

sherlock-admin2 commented 11 months ago

Escalate

I still don’t see how its Medium severity. It is users responsibility to verify that they are funding the correct pool. Even with the fix implemented by sponsor, if malicious profile creators intend to scam users, they can still market themselves as ‘vitaliks’ address. IMO this falls under user input error and my discussion here is done, will let @hrishibhat decide.

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.

quentin-abei commented 11 months ago

This will be also my final say: @nevillehuang is confusing himself by saying it falls under user input error:

nevillehuang commented 11 months ago

Ser with all due respect I did not ghost you, this is my response to you, in fact i do not see any message sent by u to me. Please avoid directing negative emotions/insults because I have never done that to you. Lets keep this positive and only leave factual based opinions here. If you have any personal things to say to me please DM, its all love.

neeksec commented 11 months ago

Suggest to keep the original judging.

This issue indeed assists in scams. Since there is no direct risk to funds and it requires scam steps to steal funds from others, I think this should be considered a low risk.

quentin-abei commented 10 months ago

To add to my already written comments , here is sherlock medium issue criteria :

Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future. The more expensive the attack is for an attacker, the less likely it will be included as a Medium (holding all other factors constant). The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.

Let me explain how a malicious user can cause a loss of funds with this exploit: For the needs of this demo i will call malicious user userA.

  • UserA create a new profile with vitalik address as owner, and his own addresses as members.
  • UserA then proceed to create a new pool and set his addresses as managers in the pool.
  • UserA will now use vitalik address as a proof of legitimacy to ask others users to fund his pool.
  • Others users funds the pool of UserA thinking that they are funding a project vitalik own.
  • UserA use his managers account to withdraw any amount from the pool.
  • Done user have used vitalik address as owner to scam people's.

Because of the above steps, i consider my issue at least a valid medium. @thelostone-mc , @neeksec

I already exolained a very likely case where an attack will occur .

Also from the Sherlock docs :

How to identify a medium issue:
Causes a loss of funds but requires certain external conditions or specific states.
Breaks core contract functionality, rendering the contract useless (should not be easily replaced without loss of funds) or leading to unknown potential exploits/loss of funds. 
Ex: Unable to remove malicious user/collateral from the contract.
A material loss of funds, no/minimal profit for the attacker at a considerable cost

My issue tick all the boxes, it should be a medium.

MLON33 commented 10 months ago

https://github.com/allo-protocol/allo-v2/pull/354

Evert0x commented 10 months ago

Planning to keep issue closed.

The issue described is not a solidity issue, it's a user issue.

Similar situation, I can create a Uniswap pool and remove all LP tokens. This is not a bug in Uniswap.

quentin-abei commented 10 months ago

Planning to keep issue closed.

The issue described is not a solidity issue, it's a user issue.

Similar situation, I can create a Uniswap pool and remove all LP tokens. This is not a bug in Uniswap.

The issue you are trying to compare this issue with does simply not match . On uniswap you can not create a pool for another address and add some members to your pool that have special permissions . If you were able to do this it would be an issue . Here you are able to do it : You can create a profile for another person and add some members that have special permissions to open a pool for the another person, grant themselves manager role to withdraw any funds sent to the pool. Maybe i speak english very poorly that only sponsor Can understand ( confirm issue, fix it , and ack a medium severity) and not Sherlock judges. Sponsor fix allows for example you to create a profile for me but you can't add members to that profile , only me can do it . Why ? because if you could add members to the profile you created for me it would allows you to add some malicious members that will then open a pool on my behalf, set themselves as managers and withdraw any funds sent to that pool. Allo is set in such a way that it's for "donations" and people's mainly donate to the pool upon seing the pool owner . I'am done with comments here , i can't understand how sherlock is trying to dismiss an issue the sponsor does not consider an acceptable risk .

Evert0x commented 10 months ago

The issue is not a smart contract issue, but a business logic issue.

Evert0x commented 10 months ago

Result: Invalid Unique

sherlock-admin2 commented 10 months ago

Escalations have been resolved successfully!

Escalation status:

quentin-abei commented 10 months ago

Since Sherlock is reconsidering it's "final" judgement see here it means it's not "final" anymore and my issue should be reconsidered for a medium at least based on the above comment on discord from jack himself. photo_2023-11-02_00-45-59 Jack comments suggest my issue have been judged based on a two way 50% by chance rule, and this brick fairness during escalations and definitely makes this judgement biased ( based on reason provided by @Evert0x to invalidate my issue. @jack-the-pug , i wrote this same comment on discord yesterday and it was deleted by someone at sherlock(if it's not you then it's someone else=. I'am urging sherlock to reconsider validating my issue since there is no factual reason provided for invalidating it, so it should be valid medium as sponsor requested.