Implicit vs. Explicit state resolution

[andreiashu]

Description

The Elrond Launchpad contract LaunchStage selection relies on the get_launch_stage function which returns the current stage:

review-elrond-launchpad-2021-10/code/launchpad/src/launchpad.rs

Lines 379 to 380 in a32fe0b

	#[view(getLaunchStage)]
	fn get_launch_stage(&self) -> LaunchStage {

This function is used to restrict actions only to specific stages:

review-elrond-launchpad-2021-10/code/launchpad/src/launchpad.rs

Lines 292 to 294 in a32fe0b

	#[endpoint(claimLaunchpadTokens)]
	fn claim_launchpad_tokens(&self) -> SCResult<()> {
	self.require_stage(LaunchStage::Claim)?;

The logic determines the current stage dynamically, based on the value of multiple state variables:

• self.nr_winning_tickets()
• self.total_confirmed_tickets()
• self.claim_start_epoch()
• self.winner_selection_start_epoch()
• self.current_generation()
• self.confirmation_period_start_epoch()
• self.confirmation_period_in_epochs()

Some of these variables can be changed by the owner at will (with some limitations).

The issue is that the logic that determines the current stage makes use of dynamic variables that can be and are changed during the lifecycle of this contract:

review-elrond-launchpad-2021-10/code/launchpad/src/launchpad.rs

Lines 383 to 397 in a32fe0b

	let total_winning_tickets = self.nr_winning_tickets().get();
	let total_confirmed_tickets = self.get_total_confirmed_tickets();
	if total_confirmed_tickets >= total_winning_tickets {
	let claim_start_epoch = self.claim_start_epoch().get();
	if current_epoch >= claim_start_epoch {
	return LaunchStage::Claim;
	} else {
	return LaunchStage::WaitBeforeClaim;
	}
	}
	let winner_selection_start_epoch = self.winner_selection_start_epoch().get();
	if current_epoch < winner_selection_start_epoch {
	return LaunchStage::AddTickets;
	}

Compared to an explicit logic whereby the owner can manage the stage transitions manually (and the contract code validates that each transition is valid), the current version of the code is more prone to errors. An invalid state transition (see #5) was already found but was difficult to spot because of the way the get_launch_stage function uses dynamic variables that can be changed in other parts of the contract.

The documentation features a state diagram that outlines the transition flow of stages:

Below is an example of how this flow chart does not necessarily represent what the code can do:

1. Current stage is ConfirmTickets (ie. confirmation_period_start_epoch < current_epoch < confiration_period_end_epoch)
2. Owner updates the following state variables so that current_epoch < winner_selection_start_epoch: winner_selection_start_epoch, confirmation_period_start_epoch
3. Current stage is AddTickets - the code allowed the owner to perform a transition from ConfirmTickets to AddTickets which is not reflected in the flowchart
4. Further on, this invalid transition can pose a risk since adding more tickets in its current state might expose other issues

Recommendation

We prefer a more explicit way of managing the state. If resources allow we recommend rewriting the get_launch_stage function to allow the owner to set the current stage and only do sanity checks to ensure that the state transition is valid.