Badger Citadel contest - pauliax's results

Lines of code

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

Vulnerability details

Impact

I thought of a potential rug pull from the owner: when users buy tokenOut, it is not required that the contract has already escrowed enough tokenOut. It is only required when finalizing the sale:

  require(
      tokenOut.balanceOf(address(this)) >= totalTokenOutBought,
      "TokenSale: not enough balance"
  );

This doesn't look fair from the perspective of users. Users have to trust that after the sale the owner will send enough tokens and finalize the sale. This increases the trust of one entity, while the goal of smart contracts should be to minimize this. When designing a smart contract, it should aim for as fair model as possible. Now the owner of the contract is privileged, the saleRecipient instantly receives tokenIn, but users will have to wait till the owner finalizes the sale.

I thought if this should be submitted as of low severity, but because this is explicitly mentioned in the README: "Specific care should be put in: Rug Vectors" I decided to submit this as of medium severity.

Recommended Mitigation Steps

Suggestions:

1. Escrow tokenOut upfront, verify contract holds enough when a user buys the token.
2. Allow anyone to finalize the sale once the sale ends, or even replace this function with a view that returns true when the sale has ended.
3. After the sale, an owner can sweep unsold tokenOut.

Lines of code

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

Vulnerability details

Impact

Users should be able to control the accepted price. The owner can anytime invoke function setTokenOutPrice and thus change the ratio of token in/out. Users have to trust the owner not to front-run them and make the tokens more expensive.

Recommended Mitigation Steps

It would be fair if users can specify the (min) price, or (min) token out the amount that they are happy with when calling the buy function, so in case it is updated before the tx is mined, it will not execute the buy.

• Open todos in the codebase, e.g.:

 TODO: Better revert strings

• Consider introducing a claim deadline. after the deadline, unclaimed tokens can be swept or burned by the owner. This would prevent tokens from being stuck forever in the contract in case a user is unavailable to claim them.

• Consider introducing reasonable upper limits for saleStart and saleDuration. This would further reduce the trust in the owner, who can grief by e.g. calling setSaleStart very far in the future, or setSaleDuration too long for our generation to finish, thus making users never receive their tokens. Think of reasonable values, for instance, saleStart max 1 month in the future, and when updating validate it against the initial value, and saleDuration max 1 year, or something like that. Or these values could be configurable on initialization but should be transparent (users can validate).

• The comment says that it pauses the sale:

  * @notice Pause the sale. Can only be called by owner
  function pause() external onlyOwner

But whenNotPaused modifier is also applied to the function claim which is invokable after the sale has been finalized. This again increases the trust of the owner, who can pause the claims leaving users no choice to claim their tokens. However, I also understand that from your point of view, this would leave no option to stop claims in case of an emergency situation. So submitting this FYI to decide if changes are necessary here.

• function setTokenOutPrice should also validate !finalized. While this does not cause any harm, it makes it more coherent if the price can't be updated once the sale is finalized.

• function buy can backrun initialize when saleStart = block.timestamp:

  require(saleStart <= block.timestamp, "TokenSale: not started");

In my opinion, it would be better to require _saleStart > block.timestamp in initialize function so that all the buys can't come in the same block. But I do not see any direct harm of this, so it is just a suggestion.

• It does not validate that a _daoId exists, and the user choice can't be changed even in the case of a non-existent id. Or if the id is not specified, it will be favoring a default of 0 id dao. I see 2 possible improvements here:

1. Allow the users to change daoVotedFor for all their boughtAmounts. Maybe apply other restrictions, e.g. can be changed until the sale is finalized.
2. Introduce a dao registry, where valid ids of DAOs could be stored.

• This can be cached to avoid external call and re-calcultation every time unless it was intended in case the token decimals might change:

  10**tokenOut.decimals()

• 'unchecked' directive can be applied to math operations where overflow/underflow can't happen:

    if (totalTokenIn < tokenInLimit) {
        limitLeft_ = tokenInLimit - totalTokenIn;
    }

• Would avoid duplicate calculation if you replace this:

      require(
          totalTokenIn + _tokenInAmount <= tokenInLimit,
          "total amount exceeded"
      );
     ...
     totalTokenIn += _tokenInAmount;

with:

    totalTokenIn += _tokenInAmount; 
    require(
        totalTokenIn <= tokenInLimit,
        "total amount exceeded"
    );
   ...

• saleStart state variable is read twice here, better cache it to a local variable and re-use:

  require(saleStart <= block.timestamp, "TokenSale: not started");
  require(
      block.timestamp < saleStart + saleDuration,
      "TokenSale: already ended"
  );