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

possible reentrancy attacks in refund function

anjalit

Impact

The root of the problem are in refund who makes a call to refund the entrance fee back to player. With this call the recipient can execute an reentrancy attack calling several times the sendValue function to steal funds and take advantage of the protocol.

Vulnerability Details

The implementation of refund does not strictly follow the Checks-Effects-Interactions (CEI) pattern as it is updating the address after sending out the entranceFee.
L96-L105 uses sendValue function from Address.sol which clearly says as the control is transferred to 'recipient' care must be taken to not create reentrancy vulnerabilities.

/**
* @dev Replacement for Solidity's `transfer`: sends `amount` wei to
* `recipient`, forwarding all available gas and reverting on errors.
*
* https://eips.ethereum.org/EIPS/eip-1884[EIP1884] increases the gas cost
* of certain opcodes, possibly making contracts go over the 2300 gas limit
* imposed by `transfer`, making them unable to receive funds via
* `transfer`. {sendValue} removes this limitation.
*
* https://diligence.consensys.net/posts/2019/09/stop-using-soliditys-transfer-now/[Learn more].
*
* IMPORTANT: because control is transferred to `recipient`, care must be
* taken to not create reentrancy vulnerabilities. Consider using
* {ReentrancyGuard} or the
* https://solidity.readthedocs.io/en/v0.5.11/security-considerations.html#use-the-checks-effects-interactions-pattern[checks-effects-interactions pattern].
*/
function sendValue(address payable recipient, uint256 amount) internal {
require(address(this).balance >= amount, "Address: insufficient balance");
​
// solhint-disable-next-line avoid-low-level-calls, avoid-call-value
(bool success, ) = recipient.call{ value: amount }("");
require(success, "Address: unable to send value, recipient may have reverted");
}

internally sendValue uses call function to send entranceFee back to the player which brings a possiblity to make a reentrancy attack . If the player is not an EOA but a contract and is set up to immediately call refund again as part of its fallback function. Since the state of the players array hasn't been updated yet (the attacker's address hasn't been set to address(0)), contract thinks attacker is still a valid player and sends the entrance fee again. This can be repeated as many times as the attacker wants (or until the contract runs out of gas), draining the contract of its funds.

Tool Used

Manual Review

Recommended Mitigation

  • One possibility is to add nonReentrant guard to refund function.

  • Another, Implement the CEI pattern in refund function by updating player address to address(0) before making external calls.

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");
​
players[playerIndex] = address(0);
payable(msg.sender).sendValue(entranceFee);
​
emit RaffleRefunded(playerAddress);
}
Updates

Lead Judging Commences

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

reentrancy-in-refund

reentrancy in refund() function

Support

FAQs

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

Give us feedback!