First Flight #17: Dussehra

Summary

Reentrancy in Dussehra.sol::killRavana means organiser can take all the money by calling Dussehra.sol::killRavana twice in a row

Vulnerability Details

The organiser decides when the ram is chosen as choosing ram through ChoosingRam.sol::selectRamIfNotSelected is the only way to set ChoosingRam.sol::isRamSelected to true which allows ravana to be killed and proceeds to be withdrawn by ram as . Therefore, the organiser can call the onlyOrganiser function ChoosingRam.sol::selectRamIfNotSelected, then in the same transcation call Dussehra.sol::killRavana twice so that the actual ram has no opportunity to withdraw the proceeds that belong to ram before half the funds are sent to the organiser twice. This is a possible reentrancy attack but also it can be done in two transactions as killing ravana and sending half the funds to the organiser but doesnt prevent Dussehra.sol::killRavana from being called successfully a second time before ram calls withdraw as there is no check to see if ravan has already been killed.

Proof of code

Add this test to Dussehra.t.sol. This test demonstrates that the organiser can take all the funds before ram can take the half the belongs to ram.

Impact

The organiser can steal all the funds in one transaction. It is also possible to steal it in two transactions

Tools Used

Manual review

Recommendations

1. You should add a RavanNotKilled modifier that only allows Dussehra.sol::killRavana to be called if ravan hasn't been killed. This ensure that after the first successful call of Dussehra.sol::killRavana, Dussehra.sol::IsRavanKilled will be true, ensuring it can never be called again as there is no other way to set it to false.

2. Add a nonReentrant modifier from openzeppelin to the Dussehra.sol::killRavana function. Although this is not necessary as CEI is followed and Dussehra.sol::IsRavanKilled is set to true before funds are sent to the organiser and the new Dussehra.sol::RavanNotKilled modifier will prevent any reentrancy, it is still best practice to use nonReentrant modifier just to be safe.