Badger Citadel contest - hickuphh3's results

Lines of code

https://github.com/code-423n4/2022-02-badger-citadel/blob/main/contracts/TokenSaleUpgradeable.sol#L251-L265

Vulnerability details

Impact

Sale participants will only be able to claim their CTDL tokens once the sale is finalized. However, there is no guarantee that it ever will be, because:

• Sale finalisation can only be performed by the owner
• The owner is able to change the sale parameters (sale start time, duration, tokenInLimit) anytime before the finalization of the sale, such that saleEnded() will always be false. Eg. setting long sale duration or a very distant future sale start time.
• There is no guarantee that the contract will have sufficient CTDL for the finalization of the sale.

function finalize() external onlyOwner {
  require(!finalized, "TokenSale: already finalized");
  require(saleEnded(), "TokenSale: not finished");
  require(
      tokenOut.balanceOf(address(this)) >= totalTokenOutBought,
      "TokenSale: not enough balance"
  );

  finalized = true;

	emit Finalized();
}

The core issue is that users’ funds are immediately accessible by the sale recipient at the moment of purchase while they have to wait for the sale to be complete before receiving their tokens. From a sale participant’s POV, he will feel that the organization can at anytime choose not to fulfil their end of the sale (ie. get rugged).

Recommended Mitigation Steps

1. Only allow critical sale parameters (especially start time) to be changed before the start of the token sale.

// have following require statement in setSaleStart()
// possibly in setSaleDuration() and setTokenInLimit() as well
// but it might be intended to have the sale extended / ended prematurely
require(block.timestamp < saleStart, "TokenSale: sale has already begun");

1. Have a sanity limit on the sale duration

uint64 internal LIMIT = 1 year;
// in setSaleDuration()
require(_saleDuration < LIMIT, "TokenSale: exceed limit");

1. Allow sale finalisation to be permissionless; remove the onlyOwner restriction.

Scenario 1: CTDL tokens can be transferred before sale commences

While token claiming can still be done after the sale, a check can be added to ensure the CTDL balance of the contract is at least the totalTokenOutBought in the purchase.

function buy() {
	...
totalTokenOutBought += tokenOutAmount_;
require(tokenOut.balanceOf(address(this)) >= totalTokenOutBought, "insufficient balance");
	...
}

Scenario 2: CTDL tokens can only be transferred after sale ends

In this less ideal scenario, have the funds transferred to an escrow contract. Should users not receive CTDL tokens after a certain time, users are entitled to reclaim their funds.