-
Notifications
You must be signed in to change notification settings - Fork 2
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
1 parent
90a7d41
commit 65ac2dd
Showing
617 changed files
with
46,017 additions
and
10 deletions.
There are no files selected for viewing
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
Merry Sepia Whale | ||
|
||
High | ||
|
||
# ETH raffle winners can steal funds from the Prize Manager | ||
|
||
## Summary | ||
Given that a winner is entitled to an amount put up for a raffle after the raffle ends and can make a claim to retrieve the amount, winners of ETH token raffles can steal funds from the prize manager. | ||
|
||
## Vulnerability Detail | ||
Issue arises from the fact that the claim status is mutated after the ether transfer and there is no non-reentrant modifier since the code doesn't follow CEI. | ||
|
||
Step to exploit this issue: | ||
1. Assuming the winner won an ETH raffle, and there is more than one ETH raffle running concurrently with the prize of each raffle being 1 ETH each, (total ETH balance of contract being 2 ETH) they can use reentrancy to get all 2 ETH in the contract in one `claimPrize` transaction when they should only get 1 ETH. | ||
2. The attacker needs to ensure they have the same contract across Avalanche (where tickets are bought) and Ethereum (where prizes are claimed). This is easily achievable as they can deploy the same contract on both chains to get the same address. To demonstrate, I have gone ahead to deploy a contract with the same address across Ethereum Sepolia and Polygon Amoy. Addresses are: Sepolia - https://sepolia.etherscan.io/address/0x13ac81ec680633f815241949126190ccadbcf07c, Polygon Amoy - https://www.oklink.com/amoy/address/0x13ac81ec680633f815241949126190ccadbcf07c | ||
|
||
```solidity | ||
function claimPrize(uint256 raffleId) external { | ||
RafflePrize storage rafflePrize = _rafflePrize[raffleId]; | ||
RaffleType raffleType = rafflePrize.raffleType; | ||
if (raffleType == RaffleType.NFT) { | ||
NFTInfo storage raffle = _nftRaffles[raffleId]; | ||
_nftLocked[raffle.contractAddress][raffle.tokenId] = false; | ||
_sendNFTPrize(raffle.contractAddress, raffle.tokenId, msg.sender); | ||
} else if (raffleType == RaffleType.TOKEN) { | ||
TokenInfo storage raffle = _tokenRaffles[raffleId]; | ||
unchecked { _tokensLocked[raffle.tokenAddress] -= raffle.amount; } | ||
_sendTokenPrize(raffle.tokenAddress, raffle.amount, msg.sender); | ||
} else if (raffleType == RaffleType.ETH) { | ||
unchecked { _ethLocked -= _ethRaffles[raffleId]; } | ||
_sendETHPrize(_ethRaffles[raffleId], msg.sender); | ||
} else revert InvalidRaffle(); | ||
if (msg.sender != rafflePrize.winner) revert UnauthorizedToClaim(); | ||
if (rafflePrize.status == RafflePrizeStatus.CLAIMED) revert AlreadyClaimed(); | ||
@> rafflePrize.status = RafflePrizeStatus.CLAIMED; // @audit done too late | ||
emit PrizeClaimed(raffleId, msg.sender); | ||
} | ||
``` | ||
|
||
## Impact | ||
Loss of all ethers in the contract at the point of exploit belonging to other ETH raffles that is prizes locked in the `WinnablesPrizeManager` contract. | ||
|
||
## Code Snippet | ||
https://github.com/sherlock-audit/2024-08-winnables-raffles/blob/main/public-contracts/contracts/WinnablesPrizeManager.sol#L105-L123 | ||
|
||
## Tool used | ||
Manual Review | ||
|
||
## Recommendation | ||
Set the `rafflePrize.status` to CLAIMED before the ether transfer to the user. Ideally, before the if statements. Also throw in a non-reentrant modifier: | ||
|
||
```diff | ||
function claimPrize(uint256 raffleId) external { | ||
RafflePrize storage rafflePrize = _rafflePrize[raffleId]; | ||
RaffleType raffleType = rafflePrize.raffleType; | ||
+ if (msg.sender != rafflePrize.winner) revert UnauthorizedToClaim(); | ||
+ if (rafflePrize.status == RafflePrizeStatus.CLAIMED) revert AlreadyClaimed(); | ||
+ rafflePrize.status = RafflePrizeStatus.CLAIMED; | ||
... | ||
- if (msg.sender != rafflePrize.winner) revert UnauthorizedToClaim(); // move to the top | ||
- if (rafflePrize.status == RafflePrizeStatus.CLAIMED) revert AlreadyClaimed(); // move this to the top | ||
- rafflePrize.status = RafflePrizeStatus.CLAIMED; // move this from the end to the top | ||
} | ||
``` |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
Proud Pistachio Tardigrade | ||
|
||
High | ||
|
||
# Reentrancy in claimPrize() at WinnablesPrizeManager.sol | ||
|
||
### Summary | ||
|
||
|
||
The claimPrize() function in the WinnablesPrizeManager.sol contract is vulnerable to reentrancy. The vulnerability is located in the logic that handles the transfer of different types of prizes (NFTs, tokens, and ETH) before updating the internal state of the contract. An attacker could exploit this vulnerability to repeatedly call the claimPrize function through the callback from the ETH transfer and drain the contract. | ||
|
||
https://github.com/sherlock-audit/2024-08-winnables-raffles/blob/main/public-contracts/contracts/WinnablesPrizeManager.sol#L105-L124 | ||
|
||
File: WinnablesPrizeManager.sol | ||
|
||
105: function claimPrize(uint256 raffleId) external { //@audit reentrancy | ||
106: RafflePrize storage rafflePrize = _rafflePrize[raffleId]; | ||
107: RaffleType raffleType = rafflePrize.raffleType; | ||
.. | ||
116: } else if (raffleType == RaffleType.ETH) { | ||
117: unchecked { _ethLocked -= _ethRaffles[raffleId]; } | ||
118: _sendETHPrize(_ethRaffles[raffleId], msg.sender); | ||
119: } else revert InvalidRaffle(); | ||
120: if (msg.sender != rafflePrize.winner) revert UnauthorizedToClaim(); | ||
121: if (rafflePrize.status == RafflePrizeStatus.CLAIMED) revert AlreadyClaimed(); | ||
122: rafflePrize.status = RafflePrizeStatus.CLAIMED; | ||
123: emit PrizeClaimed(raffleId, msg.sender); | ||
124: } | ||
|
||
|
||
### Root Cause | ||
|
||
The function doesn't follow the checks-effects-interactions pattern and doesn't have a reentrancy guard nonReentrant modifier in place. | ||
|
||
### Internal pre-conditions | ||
|
||
_No response_ | ||
|
||
### External pre-conditions | ||
|
||
_No response_ | ||
|
||
### Attack Path | ||
|
||
1. The attacker calls claimPrize, initiating a prize claim. | ||
|
||
2. Before the contract updates the rafflePrize.status to CLAIMED, the attacker triggers a fallback function in the recipient contract that re-enters claimPrize. | ||
|
||
3. The reentrant call proceeds to claim the same prize again, as the internal state indicating the prize has been claimed (rafflePrize.status) has not yet been updated. | ||
|
||
4.This process can be repeated, allowing the attacker to drain the contract of its assets. | ||
|
||
### Impact | ||
|
||
An attacker can drain the contract of its assets. | ||
|
||
### PoC | ||
|
||
|
||
|
||
### Mitigation | ||
|
||
1. Refactor the function to follow the checks-effects-interactions pattern. Ensure all state changes are made before external calls are performed. | ||
|
||
2. Add a reentrancy guard modifier to the function. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
Sour Denim Alpaca | ||
|
||
High | ||
|
||
# Multiple Reentrancy Vulnerabilities at WinnablesPrizeManager contract | ||
|
||
### Summary | ||
|
||
Functions like _sendETHPrize, _sendTokenPrize, and _sendNFTPrize make external calls to transfer ETH, ERC20 tokens, or NFTs to a winner without implementing any reentrancy guards. If an attacker gains control over the winner address, they could potentially re-enter the contract and exploit the state before it is updated. | ||
Line: https://github.com/sherlock-audit/2024-08-winnables-raffles/blob/main/public-contracts/contracts/WinnablesPrizeManager.sol#L314 | ||
|
||
|
||
### Root Cause | ||
|
||
reentrancy guards missing. | ||
|
||
### Internal pre-conditions | ||
|
||
_No response_ | ||
|
||
### External pre-conditions | ||
|
||
_No response_ | ||
|
||
### Attack Path | ||
|
||
_No response_ | ||
|
||
### Impact | ||
|
||
This could allow an attacker to drain the contract's funds or manipulate the contract state in an unintended manner. | ||
|
||
### PoC | ||
|
||
Malicious Contract | ||
This contract will attempt to exploit the sendETHPrize function by reentering the claimPrize function to drain all ETH from the WinnablesPrizeManager contract. | ||
|
||
```solidity | ||
pragma solidity 0.8.24; | ||
import "./WinnablesPrizeManager.sol"; | ||
contract MaliciousContract { | ||
WinnablesPrizeManager public target; | ||
uint256 public raffleId; | ||
constructor(WinnablesPrizeManager _target, uint256 _raffleId) { | ||
target = _target; | ||
raffleId = _raffleId; | ||
} | ||
// Fallback function will be triggered during the ETH transfer and will reenter the claimPrize function | ||
receive() external payable { | ||
if (address(target).balance > 0) { | ||
target.claimPrize(raffleId); // Reentering the target contract | ||
} | ||
} | ||
// Attack function to start the reentrancy attack | ||
function attack() external { | ||
target.claimPrize(raffleId); | ||
} | ||
} | ||
``` | ||
|
||
|
||
### Mitigation | ||
|
||
Use a nonReentrant modifier or implement reentrancy protection using state variables to ensure the function cannot be re-entered. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,117 @@ | ||
Rural Eggplant Frog | ||
|
||
Invalid | ||
|
||
# High Risk: Reentrancy Vulnerability in claimPrize | ||
|
||
### Summary | ||
|
||
The `claimPrize` function in the WinnablesPrizeManager contract is vulnerable to reentrancy attacks. This function performs external calls to transfer prizes (ETH, ERC20 tokens, or NFTs) before updating the contract's state, potentially allowing an attacker to recursively call the function and claim prizes multiple times. | ||
|
||
### Root Cause | ||
|
||
The `claimPrize` function follows a pattern that is susceptible to reentrancy: | ||
1. It checks the raffle status and winner. | ||
2. It transfers the prize (which involves external calls). | ||
3. It updates the raffle status to CLAIMED. | ||
|
||
```solidity | ||
function claimPrize(uint256 raffleId) external { | ||
RafflePrize storage rafflePrize = _rafflePrize[raffleId]; | ||
RaffleType raffleType = rafflePrize.raffleType; | ||
if (raffleType == RaffleType.NFT) { | ||
NFTInfo storage raffle = _nftRaffles[raffleId]; | ||
_nftLocked[raffle.contractAddress][raffle.tokenId] = false; | ||
_sendNFTPrize(raffle.contractAddress, raffle.tokenId, msg.sender); | ||
} else if (raffleType == RaffleType.TOKEN) { | ||
TokenInfo storage raffle = _tokenRaffles[raffleId]; | ||
unchecked { _tokensLocked[raffle.tokenAddress] -= raffle.amount; } | ||
_sendTokenPrize(raffle.tokenAddress, raffle.amount, msg.sender); | ||
} else if (raffleType == RaffleType.ETH) { | ||
unchecked { _ethLocked -= _ethRaffles[raffleId]; } | ||
_sendETHPrize(_ethRaffles[raffleId], msg.sender); | ||
} else revert InvalidRaffle(); | ||
if (msg.sender != rafflePrize.winner) revert UnauthorizedToClaim(); | ||
if (rafflePrize.status == RafflePrizeStatus.CLAIMED) revert AlreadyClaimed(); | ||
rafflePrize.status = RafflePrizeStatus.CLAIMED; | ||
emit PrizeClaimed(raffleId, msg.sender); | ||
} | ||
``` | ||
|
||
This order of operations could allow a malicious contract to re-enter the `claimPrize` function before the raffle status is updated to CLAIMED, potentially leading to multiple prize claims for the same raffle. | ||
|
||
The vulnerable pattern is as follows: | ||
1. Check conditions | ||
2. Transfer prize (external call) | ||
3. Update state | ||
|
||
An attacker could exploit this by creating a malicious contract with a fallback function that calls `claimPrize` again when it receives ETH or tokens. | ||
|
||
### Internal pre-conditions | ||
|
||
_No response_ | ||
|
||
### External pre-conditions | ||
|
||
_No response_ | ||
|
||
### Attack Path | ||
|
||
_No response_ | ||
|
||
### Impact | ||
|
||
If successfully exploited, this vulnerability could lead to: | ||
- Multiple claims of the same prize | ||
- Draining of ETH or tokens from the contract | ||
- Theft of NFT prizes | ||
- Inconsistent state between prize distribution and raffle status | ||
- Potential breakdown of the entire raffle and prize distribution mechanism | ||
|
||
The severity is high due to the potential for direct financial loss and compromise of the core functionality of the contract. | ||
|
||
### PoC | ||
|
||
_No response_ | ||
|
||
### Mitigation | ||
|
||
To mitigate this vulnerability, implement the checks-effects-interactions pattern and consider using OpenZeppelin's ReentrancyGuard: | ||
|
||
Move all state updates before any external calls. | ||
Add the nonReentrant modifier to the claimPrize function. | ||
``` | ||
import "@openzeppelin/contracts/security/ReentrancyGuard.sol"; | ||
contract WinnablesPrizeManager is ReentrancyGuard { | ||
//other codes | ||
function claimPrize(uint256 raffleId) external nonReentrant { | ||
RafflePrize storage rafflePrize = _rafflePrize[raffleId]; | ||
RaffleType raffleType = rafflePrize.raffleType; | ||
if (msg.sender != rafflePrize.winner) revert UnauthorizedToClaim(); | ||
if (rafflePrize.status == RafflePrizeStatus.CLAIMED) revert AlreadyClaimed(); | ||
// Update state first | ||
rafflePrize.status = RafflePrizeStatus.CLAIMED; | ||
if (raffleType == RaffleType.NFT) { | ||
NFTInfo storage raffle = _nftRaffles[raffleId]; | ||
_nftLocked[raffle.contractAddress][raffle.tokenId] = false; | ||
_sendNFTPrize(raffle.contractAddress, raffle.tokenId, msg.sender); | ||
} else if (raffleType == RaffleType.TOKEN) { | ||
TokenInfo storage raffle = _tokenRaffles[raffleId]; | ||
unchecked { _tokensLocked[raffle.tokenAddress] -= raffle.amount; } | ||
_sendTokenPrize(raffle.tokenAddress, raffle.amount, msg.sender); | ||
} else if (raffleType == RaffleType.ETH) { | ||
unchecked { _ethLocked -= _ethRaffles[raffleId]; } | ||
_sendETHPrize(_ethRaffles[raffleId], msg.sender); | ||
} else revert InvalidRaffle(); | ||
emit PrizeClaimed(raffleId, msg.sender); | ||
} | ||
} |
Oops, something went wrong.