sherlock-audit / 2024-04-titles-judging

9 stars 6 forks source link

0x73696d616f - Referral system can be gamed as editions and works are free to chose any `referrer` they want #405

Closed sherlock-admin4 closed 4 months ago

sherlock-admin4 commented 5 months ago

0x73696d616f

high

Referral system can be gamed as editions and works are free to chose any referrer they want

Summary

Referrals send a share of the protocol fee, so they should not be chosen arbitrarly by users or the protocol risks losing yield without benefitting from having real referrals.

Vulnerability Detail

In TitlesCore::createEdition(), referrer_ is sent as an argument, so the creator of the edition is able to set the referrer to an address on its control. Edition::mint() and Edition::mintWithComment() receive the referrer_ as arguments, so the buyer can set it to an address it controls.

Impact

Loss of fees for the protocol and lack of benefit from the referral system.

Code Snippet

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/TitlesCore.sol#L72 https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/editions/Edition.sol#L232 https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/editions/Edition.sol#L256

Tool used

Manual Review

Vscode

Recommendation

Users should send a signature signed by a trusted wallet of the Titles protocol confirming that they are actually using a referral. This would make it such that the protocol would only lose fees to referrals if they were actually used.

0x73696d616f commented 4 months ago

Escalate

This should be a valid high issue, the referrer system can be made useless.

sherlock-admin3 commented 4 months ago

Escalate

This should be a valid medium issue, the referrer system can be made useless.

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.

WangSecurity commented 4 months ago

Agree with the escalation, planning to accept and duplicate with #267

0x73696d616f commented 4 months ago

Hi @WangSecurity, thank you for your review, but this issue is not a duplicate of #267. My issue #220 is the duplicate of #267. This one is different.

realfugazzi commented 4 months ago

There is no issue here. Referrers are usually taken as plain argument, which simply depend on the caller. These are typically filled by a UI or front-end.

0x73696d616f commented 4 months ago

Referrers are usually taken as plain argument, which simply depend on the caller. These are typically filled by a UI or front-end.

Agreed. Problem is about the non referrers. These can steal the referrer fee. Real referrers -> no issue. Fake referrers that receive the fee for themselves -> exploit.

Hence the signature from the owner of the frontend, as in the recommendation.

realfugazzi commented 4 months ago

Referrers are usually taken as plain argument, which simply depend on the caller. These are typically filled by a UI or front-end.

Agreed. Problem is about the non referrers. These can steal the referrer fee. Real referrers -> no issue. Fake referrers that receive the fee for themselves -> exploit.

Hence the signature from the owner of the frontend, as in the recommendation.

The referrer is that one, the argument, by design. There is no real or fake.

WangSecurity commented 4 months ago

Agree with escalation that the it's possible to save funds by putting themselves as referrers and steal them from actual referrers. To my knowledge, it's unkown if it's actually checked via front-end, hence, planning to accept the escalation and make a new family of issues with high severities. Duplicates will be:

39, #147

realfugazzi commented 4 months ago

Agree with escalation that the it's possible to save funds by putting themselves as referrers and steal them from actual referrers. To my knowledge, it's unkown if it's actually checked via front-end, hence, planning to accept the escalation and make a new family of issues with high severities. Duplicates will be:

39, #147

This is the natural way referrals work, and there plenty other protocols that behave the same. There is no way to setup an open referral system without the friction introduced by the recommendations proposed in these reports.

This is a design decision and should not be posed as an issue. I recommend asking the protocol team before moving forward.

0x73696d616f commented 4 months ago

This is the natural way referrals work

How is giving a fee to users who were not referred and did not interact through a referral the 'natural way'?

There is no way to setup an open referral system without the friction introduced by the recommendations proposed in these reports.

Users should send a signature signed by a trusted wallet of the Titles protocol confirming that they are actually using a referral. Frontend -> backend signs -> frontend -> user referral.

This is a design decision and should not be posed as an issue. I recommend asking the protocol team before moving forward.

This does not matter, such a decision would need to have been placed in the known risks.

realfugazzi commented 4 months ago

How is giving a fee to users who were not referred and did not interact through a referral the 'natural way'?

I suggest you review how referrals work.

Users should send a signature signed by a trusted wallet of the Titles protocol confirming that they are actually using a referral. Frontend -> backend signs -> frontend -> user referral.

This defeats the purpose of having an open referral system.

This does not matter, such a decision would need to have been placed in the known risks.

It does matter as they designed the protocol.

Again, recommend asking the team.

brakeless-wtp commented 4 months ago

Other duplicates - #97 , #250 , #323 , #352 , #424

WangSecurity commented 4 months ago

@realfugazzi correct if I'm wrong here, but if the user calls contract directly and inputs themselves as a referrer, wouldn't they receive the fees?

I understand that referals work in that way in many protocols and it can be mitigated by the front-end, but if we call the contract directly bypassing front-end, isn't it possible to exploit the protocol in such way?

WangSecurity commented 4 months ago

If these questions remains unanswered, my decision remains the same, accept the escalation and validate with high severity with the following duplicates:

39, #147, https://github.com/sherlock-audit/2024-04-titles-judging/issues/97 , https://github.com/sherlock-audit/2024-04-titles-judging/issues/250 , https://github.com/sherlock-audit/2024-04-titles-judging/issues/323 , https://github.com/sherlock-audit/2024-04-titles-judging/issues/352 , https://github.com/sherlock-audit/2024-04-titles-judging/issues/424

realfugazzi commented 4 months ago

@realfugazzi correct if I'm wrong here, but if the user calls contract directly and inputs themselves as a referrer, wouldn't they receive the fees?

I understand that referals work in that way in many protocols and it can be mitigated by the front-end, but if we call the contract directly bypassing front-end, isn't it possible to exploit the protocol in such way?

Correct, but this is how referral systems work in protocols, even in web2. The idea is that the underlying contracts offer an incentive so that third party retailers could drive traffic to it. These sources would set up their address as their referrer, but of course anyone can set themselves as the referrers. This is a tradeoff for a permissionless design, imposing a whitelist or check would generate friction and demotivate referrers.

Feel free to ping the sponsor so they have the final word. IMHO this is clearly a design decision.

WangSecurity commented 4 months ago

Yeah, these are fair points, and I'm quite sure it'll be done via UI or in other ways. And I'll flag this to sponsors, but I'm quite sure they'll say it's design decision. Still, this design decision allows for using yourself as a referrer when interacting with the contract directly. They can also say it's acceptable risk, but it's wasn't a known risk during the contest (at least I don't see it). Hence, the users can use themselves as referrers without any limitations (don't see "using the contract directly" as an extensive external limitation), leading to a loss of fees to the protocol and the users paying less than they should.

Therefore, if there is no information about it being a known issue during the contest AND/OR no limitations within the contracts that prevent this issue, planning to accept the escalation with high severity and duplicates mentioned in my previous comment.

Tarunrao0 commented 4 months ago

@WangSecurity My issue #65 is also a duplicate of this issue could you please include it in the family.

sammy-tm commented 4 months ago

@WangSecurity

Not sure how this was overlooked, but #352 was incorrectly duped with a different issue while it is actually a dup of this issue.

realfugazzi commented 4 months ago

Yeah, these are fair points, and I'm quite sure it'll be done via UI or in other ways. And I'll flag this to sponsors, but I'm quite sure they'll say it's design decision. Still, this design decision allows for using yourself as a referrer when interacting with the contract directly. They can also say it's acceptable risk, but it's wasn't a known risk during the contest (at least I don't see it). Hence, the users can use themselves as referrers without any limitations (don't see "using the contract directly" as an extensive external limitation), leading to a loss of fees to the protocol and the users paying less than they should.

Therefore, if there is no information about it being a known issue during the contest AND/OR no limitations within the contracts that prevent this issue, planning to accept the escalation with high severity and duplicates mentioned in my previous comment.

There is no known issue because there is no issue. You are basically paying the fee to yourself, no attack, no asset loss, no lock, no broken behavior.

WangSecurity commented 4 months ago

The loss is that the protocol gets no fees and the users save on the fees. Why do you think it's not a loss for the protocol and not an incorrect behaviour?

realfugazzi commented 4 months ago

The loss is that the protocol gets no fees and the users save on the fees. Why do you think it's not a loss for the protocol and not an incorrect behaviour?

No, the protocol fee is different from the referral fee.

0x73696d616f commented 4 months ago

@realfugazzi the referral fee is subtracted from the protocol fee. Here

realfugazzi commented 4 months ago

@realfugazzi the referral fee is subtracted from the protocol fee. Here

Exactly, the protocol gets the same amount independently of who the referrer is. So circumventing the referrer doesn't cause a loss for the protocol.

0x73696d616f commented 4 months ago

I think you completely missed the point of this issue. Here

realfugazzi commented 4 months ago

I think you completely missed the point of this issue. Here

no, I don't think so.

Let's ping the team and confirm this.

0x73696d616f commented 4 months ago

Ok so some user goes to etherscan and mints using his address as referral. Who did this person refer to deserve receiving a fee?

WangSecurity commented 4 months ago

Sorry, I made a mistake, indeed the protocol doesn't lose anything. It's the actual referrers who lose the fees. For example, the scenario: Alice is a referrer (for example minting referrer), but Bobs calls mint directly and sets himself as a referrer. Then when it comes to _splitProtocolFee a portion of the fee would be sent back to Bob and not the actual referrer (Alice in this example). @realfugazzi if I'm missing something here, please help me understand, cause I genuinely don't really understand why do you mean no loss? The actual referrers gets no fees and the regular user (not a referrer) mints a token at a discount, cause the portion of the fee is returned to themselves? Please tell me what is missing here (besides that it can be fixed by UI, it's still possible by calling the contract directly).

0x73696d616f commented 4 months ago

the regular user (not a referrer) mints a token at a discount, cause the portion of the fee is returned to themselves

The issue is here. The referrer fee is subtracted from the protocol and goes to a regular user that did not contribute to the referral system.

ccashwell commented 4 months ago

I don't believe this is a valid issue. As @realfugazzi noted this is generic to permissionless referral systems, and it's a conscious design choice, not a logic issue. The protocol stands to only gain in this scenario. Although a portion of the protocol fee is paid to the self-referring user, the entire fee only exists in the first place because of the user's activity.

WangSecurity commented 4 months ago

@ccashwell but the actual referrers that has to get the fee, won't receive it and the user will mint tokens at a discount, or it's wrong?

Or as I understand, in any way the user can input address(0) as a referrer to not pay out the fee. And the fee has to be paid only if the minter actually wants to, but has no obligation to do that in any case?

ccashwell commented 4 months ago

The use case is for e.g. referral links to a mint page on the client application which will pass the value through to the contract if a mint occurs in the client app. If the user calls the contract directly without the referrer address, we consider that to not be referred at all.

WangSecurity commented 4 months ago

Thanks for that quick response! Then I agree it's design decision and up to the user to decide of they want to refer to anyone and pay a fee for that. Excuse me for missing this point earlier. Agree that there is no loss for the protocol, hence, planning to reject the escalation and leave the issue as it is.

0x73696d616f commented 4 months ago

@WangSecurity a user gets a discount and did not refer anyone. This is valid, unless it was a known risk.

WangSecurity commented 4 months ago

From the last sponsor's comment, specifically, these words:

If the user calls the contract directly without the referrer address, we consider that to not be referred at all

I believe means that it's user decision to refer to anyone or not. If they want to, they will pay an additional fee, if not, then they don't pay that additional fee. It's not an obligation for the user refer to anyone. Therefore, I think it's indeed not an issue and a design decision.

Decision, remains the same, reject the escalation.

Tarunrao0 commented 4 months ago

@WangSecurity This a valid issue. The protocol is supposed to get 0.0006 eth if there is no referrer. But when a user enters their own address as the referrer they get back 0.0003 eth and the protocol gets only 0.0003eth hence paying only half the mintFee. This wasnt a known risk and I believe this should be validated

eshaasg commented 4 months ago

@WangSecurity The protocol had to get 0.006 eth and gets back only 0.0003 eth when users enter their own address as the referrer. Thus paying only half the mintFee. This is a valid issue and i believe this should thus be validated.

Tarunrao0 commented 4 months ago

Sorry, I made a mistake, indeed the protocol doesn't lose anything. It's the actual referrers who lose the fees. For example, the scenario: Alice is a referrer (for example minting referrer), but Bobs calls mint directly and sets himself as a referrer. Then when it comes to _splitProtocolFee a portion of the fee would be sent back to Bob and not the actual referrer (Alice in this example). @realfugazzi if I'm missing something here, please help me understand, cause I genuinely don't really understand why do you mean no loss? The actual referrers gets no fees and the regular user (not a referrer) mints a token at a discount, cause the portion of the fee is returned to themselves? Please tell me what is missing here (besides that it can be fixed by UI, it's still possible by calling the contract directly).

@WangSecurity There is no Alice, Bob is minting with the motive of getting half the fee back by referring himself as the referrer. Consider the price of the token to mint to be 0.01 ether The protocol fee is 0.0006 ether let us assume Bob has only 0.0106 ether where 0.01 ether is sent to the creator and 0.0006 ether is sent to the protocol when there is no referrer Once minted Bob must have 0 balance.

But if Bob mentions his own address as the referrer he gets 0.0003 ether back as the referrer's share allowing Bob to pay only half the protocol fee

WangSecurity commented 4 months ago

The protocol doesn't lose any fee and it's sent either way, regardless of who is put as the referrer. Yes, the referrers won't receive the fee it the user inputs themselves or address(0) as the referrer, but it's the design decision. The user is no obligated to refer to someone. If they want to, they have an opportunity to refer, but it's not mandatory and they can mint without referring to anyone. Therefore, there is no loss per se, cause the referrers are not supposed to get the fee every time, but only in the cases if the minter wants to.

Hence, decision remains the same, planning to reject the escalation.

Evert0x commented 4 months ago

I will reject the escalation in a few hours

0x73696d616f commented 4 months ago

@WangSecurity agree that this is a design choice, but my line of reasoning and the other watson's that submitted this issue is perfectly valid. We only know that it is a design choice after the end of the contest (no docs) and it was not in the known risks. Regardless, I think in these scenarios it's up to the judge to decide.

Tarunrao0 commented 4 months ago

I agree with @0x73696d616f it wasn't specified to be a known risk and believe this should be a valid issue.

WangSecurity commented 4 months ago

I believe it's not a known risk is that it's not an issue. I understand that there it was unknown is the referral system works like that during the contest, but all the watsons had an opportunity to ask sponsors how the referral system work. Moreover, there is also no evidence that the users are obligated to refer to someone. Hence, my decision remains the same that it's design decision. The user is not obligated to refer to anyone unless they want to. Therefore, what the issue desribes is the intended behaviour of the protocol. The impact section also says that the protocol loses the fee, which we already agreed above is not the case. I understand that it wasn't known during the contest how exactly the system works, but there is also no guarantee that the referrers has to get the fee after every mint fee. Ultimately, I believe it's not a known risk, cause it's not an issue, but the intended way the protocol works.

Planning to reject the escalation and leave the issue as it is.

Tarunrao0 commented 4 months ago

@WangSecurity

The issue isnt about the user wronging the referrer by not referring them. Even if there isnt a referrer, the user can simply add their own address as the referrer and get back half of the fee they paid to the protocol as the referrrer fee.

"The impact section also says that the protocol loses the fee, which we already agreed above is not the case."

The protocol does not recieve half the fee it is supposed to. Please run this testsuite which demonstrates the protocol losing half the fee.

It has both scenarios where there is no referrer and a scenario where Alice adds her own address as the referrer

function test_exploited_referrer() public {
        address alice = makeAddr("minter");
        vm.deal(address(alice), 0.0106 ether);

        console.log("[Balance-Before]");
        console.log("minter's balance : %e", address(alice).balance);
        console.log("protocol balance : %e", address(0xc0ffee).balance);
        console.log("creator's balance : %e", address(1).balance);

        //Alice calls mint with her own address as the referrer
        vm.prank(alice);
        edition.mint{value: 0.0106 ether}(
            address(alice),
            1,
            1,
            address(alice),
            new bytes(0)
        );

        console.log("[Balance-After]");
        console.log("minter's balance : %e", address(alice).balance);
        console.log("protocol balance : %e", address(0xc0ffee).balance);
        console.log("creator's balance : %e", address(1).balance);
    }

    //This is a simulation of how a mint tx would be if alice didnt add her own address as the referrer
    function test_no_referrer() public {
        address alice = makeAddr("minter");
        vm.deal(address(alice), 0.0106 ether);

        console.log("[Balance-Before]");
        console.log("minter's balance : %e", address(alice).balance);
        console.log("protocol balance : %e", address(0xc0ffee).balance);
        console.log("creator's balance : %e", address(1).balance);

        vm.prank(alice);
        edition.mint{value: 0.0106 ether}(
            address(alice),
            1,
            1,
            address(0),
            new bytes(0)
        );

        console.log("[Balance-After]");
        console.log("minter's balance : %e", address(alice).balance);
        console.log("protocol balance : %e", address(0xc0ffee).balance);
        console.log("creator's balance : %e", address(1).balance);
    }
ccashwell commented 4 months ago

Only the expected share of the fee is paid out to Alice in your example, and that fee only exists because Alice minted. Consider that Alice could have literally used any address she controls to derive the same benefit. Hence, it's part of the referral system design.

There's an argument to be made that mintReferrer should explicitly not be allowed to be equal to a the minter address, but that's not what this issue is about (and more importantly that issue is already separately adjudicated).

0x73696d616f commented 4 months ago

@ccashwell expected is the key element, we had no docs, there are 2 valid interpretations from the code.

Tarunrao0 commented 4 months ago

Only the expected share of the fee is paid out to Alice in your example, and that fee only exists because Alice minted. Consider that Alice could have literally used any address she controls to derive the same benefit. Hence, it's part of the referral system design.

There's an argument to be made that mintReferrer should explicitly not be allowed to be equal to a the minter address, but that's not what this issue is about (and more importantly that issue is already separately adjudicated).

My issue #65 is about referrer explicitly not being allowed to be equal to the minter address

ccashwell commented 4 months ago

And several Watsons inquired about this specific code path. You had the opportunity to clarify the intent if there was ambiguity in the documentation.

0x73696d616f commented 4 months ago

And several Watsons inquired about this specific code path. You had the opportunity to clarify the intent if there was ambiguity in the documentation.

I don't agree with this because some watsons prefer not to disclose vulnerabilities before the end of the contest. Hence the readme

0x73696d616f commented 4 months ago

There have been examples of compromised sponsors in the past (not saying this is the case)

WangSecurity commented 4 months ago

@ccashwell if the user doesn't refer to anyone the protocol will get 0.000006 fee or 0.000003 fee as they would if there was someone referred?