aslanbek - Admin can prevent raffle winner from claiming their reward

[sherlock-admin2]

aslanbek

Medium

Admin can prevent raffle winner from claiming their reward

Summary

By whitelisting their address and sending a WINNER_DRAWN CCIP message to PrizeManager, Admin can steal the raffle prize, which breaks the invariant from README:

Winnables admins cannot do anything to prevent a winner from withdrawing their prize

Root Cause

In WinnablesPrizeManager, Admin can add/remove any address as CCIP counterpart at any time.

Internal pre-conditions

A raffle is able to reach REQUESTED stage (enough tickets were sold).

Attack Path

1. Admin sends NFT (or token/ETH) to PrizeManager.
2. Admin calls PrizeManager#lockNFT.
3. Admin calls TicketManager#createRaffle.
4. Raffle (ticket sale) ends successfully.
5. drawWinner is called.
6. Admin adds himself as PrizeManager's CCIP counterpart and sends WINNER_DRAWN CCIP message with his own address as winner to PrizeManager; Admin removes TicketManager from PrizeManager's CCIP counterparts.
7. Chainlink VRF request from step 5 is fulfilled.
8. TicketManager#propagate is called, which propagates Alice as the winner of the raffle, but CCIP message reverts on the destination chain with UnauthorizedCCIPSender().
9. Admin claims reward from PrizeManager.

Impact

Admin steals the raffle reward.

Mitigation

Admin should not be able to add or remove CCIP counterparts during Raffles.

[aslanbekaibimov]

Escalate

Sherlock rules:

The protocol team can use the README (and only the README) to define language that indicates the codebase's restrictions and/or expected functionality. Issues that break these statements, irrespective of whether the impact is low/unknown, will be assigned Medium severity. High severity will be applied only if the issue falls into the High severity category in the judging guidelines.

Example: The README states "Admin can only call XYZ function once" but the code allows the Admin to call XYZ function twice; this is a valid Medium

README:

The principles that must always remain true are:

• Winnables admins cannot do anything to prevent a winner from withdrawing their prize

[sherlock-admin3]

Escalate

Sherlock rules:

The protocol team can use the README (and only the README) to define language that indicates the codebase's restrictions and/or expected functionality. Issues that break these statements, irrespective of whether the impact is low/unknown, will be assigned Medium severity. High severity will be applied only if the issue falls into the High severity category in the judging guidelines.

Example: The README states "Admin can only call XYZ function once" but the code allows the Admin to call XYZ function twice; this is a valid Medium

README:

The principles that must always remain true are:

• Winnables admins cannot do anything to prevent a winner from withdrawing their prize

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.

[mystery0x]

All admin related issues have been discussed with the HOJ and the sponsor who wished the following governing statements could have been more carefully written and/or omitted:

"The protocol working as expected relies on having an admin creating raffles. It should be expected that the admin will do their job. However it is not expected that the admin can steal funds that should have ended in a raffle participant’s wallet in any conceivable way."

Much as many of the findings suggest good fixes, the conclusion is that they will all be deemed low unless it's beyond the trusted admin control.

[WangSecurity]

After additionally considering this issue, I agree that it breaks an invariant from the README, so should be valid based on the following rule:

The protocol team can use the README (and only the README) to define language that indicates the codebase's restrictions and/or expected functionality. Issues that break these statements, irrespective of whether the impact is low/unknown, will be assigned Medium severity. High severity will be applied only if the issue falls into the High severity category in the judging guidelines.

Planning to accept the escalation and validate this issue with medium severity. This report will be the main one and the duplication of the family will be based on the conceptual mistake of admins being able to steal funds. The duplicates are:

• 25

• 62

• 63

• 161

• 217

• 254

• 286

• 524

• 462

• 350

• 117

• 163

Additional duplicates:

• 289

• 613

• 228

• 595

• 100

• 98

• 86

• 348

• 414

Note: if there are any missing duplicates, please let me know.

[shaflow01]

@WangSecurity #348 is duplicates

[Brivan-26]

@WangSecurity Counterpart is a core contract that is needed for cross-chain communication, you can not prevent the admin from updating the counterpart contract (as suggested by the report and the duplicates). This report even suggests preventing the admin from updating the counterpart when the raffle is already ongoing, what if the admin sets a malicious counterpart before the raffle starts?

I believe the usage of counterpart by the admin falls under the trust assumption.

[rickkk137]

@WangSecurity #86 is dup of #277 also

[S3v3ru5]

289 is also a duplicate of this one

[0xshivay]

@WangSecurity Issue #613 is a duplicate of this issue.

[0xsimao]

228 is a dup.

[lolka8]

#595 is a dup of this

[iamnmt]

100 is a dup

[WangSecurity]

Counterpart is a core contract that is needed for cross-chain communication, you can not prevent the admin from updating the counterpart contract (as suggested by the report and the duplicates). This report even suggests preventing the admin from updating the counterpart when the raffle is already ongoing, what if the admin sets a malicious counterpart before the raffle starts? I believe the usage of counterpart by the admin falls under the trust assumption.

It's a very fair point and it would be invalid under normal circumstances, but here there's a statement in the README that an admin cannot prevent the winner from getting their prize, while this report shows that an admin can do that. Hence, it should be valid, based on the following:

The protocol team can use the README (and only the README) to define language that indicates the codebase's restrictions and/or expected functionality. Issues that break these statements, irrespective of whether the impact is low/unknown, will be assigned Medium severity. High severity will be applied only if the issue falls into the High severity category in the judging guidelines.

As for the other issues, I've added them into the previous comment into the "additional duplicates" question. Planning to accept the escalation and apply the changes in this comment.

[WangSecurity]

Result: Medium Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• aslanbekaibimov: accepted

[0xvj]

@WangSecurity #414 is a valid duplicate of this issue.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/Winnables/public-contracts/pull/22