First Flight #2: Puppy Raffle
First Flight #2
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Usafe Downcast of `fee` to `uint64` causing overflow

ciara

The maximum value of a uint64 is 18446744073709551615. Since fee is measured in Wei, 18446744073709551615 WEI is equivalent to 18.45 ETH. This means that if the totalFee that has accumulated in the contract surpasses 18.45 ETH, there will be an overflow error resulting in a very small fee balance.

Vulnerability Details

In PuppyRaffle::selectWinner on line 133-134, fee is initialized as a uint256 and then downcast to a uint64 on the subsequent line.

fee is calculated using the following logic:

uint256 fee = (totalAmountCollected * 20) / 100;

If totalAmountCollected surpasses (18.45 ETH * 100) / 20 which equates to 92.23 ETH, then fee will overflow when downcast to uint64 as it will surpass the maximum value of uint64:

totalFees = totalFees + uint64(fee);

Impact

Firstly, due to overflow in the fee, the totalFees that can be withdrawn by the feeAddress will be smaller than the true fees.

Secondly, if this happens, the value in the contract will be non-zero even if the winner is paid and the feeRecipient withdraws the totalFees. This will result in a state of DoS when calling withdrawFees() as explained in the following issue: "Unable to withdraw fees if contract balance is not equal to totalFees when no players are active".

Since the value of totalAmountCollected required to cause this overflow is 92.23 ETH, this is likely and therefore a high risk vulnerability.

Proof of Concept

Working Test Case

A new PuppyRaffle contract is initialized with an expensiveEntranceFee of 1e20 to ensure that the maximum value of the uint64 is reached.
The test enters 4 players into the raffle. The test passes if the value of the totalFee calculated without the unsafe downcast is greater than the value returned from the contract.

function test_poc_unsafeFeeDowncast() public {
address[] memory players = new address[](4);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
uint256 totalAmountCollected = expensiveEntranceFee * 4;
expensivePuppyRaffle.enterRaffle{value: totalAmountCollected}(players);
​
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
expensivePuppyRaffle.selectWinner();
uint256 fee = (totalAmountCollected * 20) / 100;
uint256 expectedFee = 0 + (fee);
console.log(expectedFee);
console.log(expensivePuppyRaffle.totalFees());
assertGe(expectedFee, expensivePuppyRaffle.totalFees());
}

Running the test yields the following output:

$ forge test --mt test_poc_unsafeFeeDowncast -vvvv
​
// output
Running 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest
[PASS] test_poc_unsafeFeeDowncast() (gas: 288555)
Logs:
80000000000000000000
6213023705161793536
​
Traces:
[292768] PuppyRaffleTest::test_poc_unsafeFeeDowncast()
├─ [121273] PuppyRaffle::enterRaffle{value: 400000000000000000000}([0x0000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000003, 0x0000000000000000000000000000000000000004])
│ ├─ emit RaffleEnter(newPlayers: [0x0000000000000000000000000000000000000001, 0x0000000000000000000000000000000000000002, 0x0000000000000000000000000000000000000003, 0x0000000000000000000000000000000000000004])
│ └─ ← ()
├─ [0] VM::warp(86402 [8.64e4])
│ └─ ← ()
├─ [0] VM::roll(2)
│ └─ ← ()
├─ [167953] PuppyRaffle::selectWinner()
│ ├─ [15] PRECOMPILE::identity{value: 320000000000000000000}()
│ │ └─ ←
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x0000000000000000000000000000000000000004, tokenId: 0)
│ └─ ← ()
├─ [0] console::f5b1bba9(000000000000000000000000000000000000000000000004563918244f400000) [staticcall]
│ └─ ← ()
├─ [376] PuppyRaffle::totalFees() [staticcall]
│ └─ ← 6213023705161793536 [6.213e18]
├─ [0] console::f5b1bba9(000000000000000000000000000000000000000000000000563918244f400000) [staticcall]
│ └─ ← ()
├─ [376] PuppyRaffle::totalFees() [staticcall]
│ └─ ← 6213023705161793536 [6.213e18]
└─ ← ()
​
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 8.30ms

As expected, the test passes, meaning that there has been an overflow of fee due to the unsafe downcast to a uint64.

Recommended Mitigation

  • Do not downcast fee to a uint64 and keep all ether values as uint256, including totalFee to ensure the maximum value is not surpassed.

  • Use OpenZeppelin's SafeMath as SafeMath is not enabled by default for Solidity versions before 0.8.0.

Tools Used

  • Forge

  • Chisel

Updates

Lead Judging Commences

Hamiltonite Lead Judge about 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

unsafe cast of fee to uint64

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!