sherlock-audit / 2024-08-winnables-raffles-judging

2 stars 1 forks source link

neko_nyaa - Admin can deny winnings by disabling the approved CCIP counterpart, causing results propagation to fail #25

Closed sherlock-admin2 closed 2 weeks ago

sherlock-admin2 commented 4 weeks ago

neko_nyaa

High

Admin can deny winnings by disabling the approved CCIP counterpart, causing results propagation to fail

Summary

Per the contest README:

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

By abusing setCCIPCounterpart, after the winner was decided, the admin can deny said winner from withdrawing their prize.

Root Cause

Each raffle, the protocol relies on the Chainlink VRF to determine a winning ticket on the Avalanche chain. When the randomness request is fulfilled, anyone can call WinnablesTicketManager.propagateRaffleWinner() to send the results to Ethereum Mainnet via a CCIP message, committing a winner and allowing them to claim the prize.

During _ccipReceive() on the receiving end, the receiving contract performs a check to see if the sender was actually the Winnables contract on the other chain:

(address _senderAddress) = abi.decode(message.sender, (address));
bytes32 counterpart = _packCCIPContract(_senderAddress, message.sourceChainSelector);
if (!_ccipContracts[counterpart]) revert UnauthorizedCCIPSender();

https://github.com/sherlock-audit/2024-08-winnables-raffles/blob/main/public-contracts/contracts/WinnablesPrizeManager.sol#L263-L265

However, at any time, admin can call setCCIPCounterpart() to remove the counterpart as a trusted address, even after a winner was decided.

function setCCIPCounterpart(
    address contractAddress,
    uint64 chainSelector,
    bool enabled
) external onlyRole(0) {
    _setCCIPCounterpart(contractAddress, chainSelector, enabled);
}

https://github.com/sherlock-audit/2024-08-winnables-raffles/blob/main/public-contracts/contracts/WinnablesPrizeManager.sol#L134

Then the admin is able to deny selected winners' winnings any time after the VRF result was returned, and before the results were propagated (e.g. before propagateRaffleWinner() is called, or before the CCIP message reaches the Ethereum Mainnet side), causing every received CCIP message to fail, and the winner is never propagated.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

  1. The admin locks a prize, and starts a raffle as normal. People starts buying tickets to enter the raffle over on Avalanche chain.
  2. The buying phase finishes, admin starts the VRF roll.
  3. The VRF roll is returned, and a winner is selected. Now anyone can call propagateRaffleWinner() to deliver the results to the Mainnet.
  4. Admin doesn't like the winner, so before the message is delivered to the Mainnet, the admin calls setCCIPCounterpart() on the Mainnet prize manager and denies and message coming from the Avalanche ticket manager.
  5. Because the message never goes through, the winner is never propagated to the prize manager, and the winner cannot claim their prize.
  6. Admin can reclaim the prize by setting another contract on Avalanche as the CCIP counterpart, and send a RAFFLE_CANCELED message to unlock the prize.

Impact

Admin is able to deny selected winners of their winnings. This breaks a core invariant defined by the protocol.

This also has no pre-conditions, and the result is that the winner is denied all their rightful winnings within a raffle.

PoC

No response

Mitigation

When the prize is locked and the raffle is created, also lock in the ticket manager address as part of the raffle parameters.

Duplicate of #277

neko-nyaa commented 2 weeks ago

Escalate. As per the README, the admin should not have a method to deny a winner their winnings. This submission shows one such method.

sherlock-admin3 commented 2 weeks ago

Escalate. As per the README, the admin should not have a method to deny a winner their winnings. This submission shows one such method.

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.

philmnds commented 2 weeks ago

Agreed with the escalation. There is a specific mention in the README about it:

The principles that must always remain true are:

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

My issues https://github.com/sherlock-audit/2024-08-winnables-raffles-judging/issues/62 and https://github.com/sherlock-audit/2024-08-winnables-raffles-judging/issues/63 are related.

johnson37 commented 1 week ago

#98 should be one dup of this finding. Please help check.

Brivan-26 commented 1 week ago

I believe this does not fit under the rule of readme saying that the admin should have no way to prevent the winner from claiming his prize. The function setCCIPCounterpart is a core, administrative function to set the contracts of other chains, I believe it is under trusted admin control

mystery0x commented 1 week ago

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 commented 5 days ago

After additionally considering this issue, the escalation will be accepted and the issue will be validated with medium severity and duplicated with #277. For more details, look into the discussion under #277.

WangSecurity commented 1 day ago

Result: Medium Duplicate of #277

sherlock-admin4 commented 1 day ago

Escalations have been resolved successfully!

Escalation status: