Badger Citadel contest - TomFrenchBlockchain's results

Lines of code

https://github.com/code-423n4/2022-02-badger-citadel/blob/84596551d62f243d13fcb2d486346dde08002f7b/contracts/TokenSaleUpgradeable.sol#L137-L205

Vulnerability details

Impact

BadgerDAO is both incentivised and capable of arbitrarily upgrading the logic of the smart contract to retain both CTDL and user's funds.

Proof of Concept

The process of buying CTDL from TokenSaleUpgradeable goes as follows:

1. Buyer deposits WBTC by calling buy
2. Sale period completes.
3. BadgerDAO calls the finalize function.
4. Buyer calls claim function to withdraw CTDL tokens.

Both the CTDL and WBTC tokens will then sit inside the TokenSaleUpgradeable contract until BadgerDAO calls the finalize function.

BadgerDAO is in a unique position with regards to this contract as they are simultaneously one of the parties to the sale as well as the admin over the TransparentUpgradeableProxy through which TokenSaleUpgradeable is made available. They may then upgrade the logic of the contract arbitrarily, allowing them to withdraw both the WBTC and CTDL instead of finalising the sale.

BadgerDAO makes usage of proxy contracts extensively so this may not be immediately considered an issue, however in this example BadgerDAO's incentives are directly opposed to that of the smart contract's users as opposed to them being in alignment as for it's vault contracts.

We can consider this contract to be equivalent to an escrow contract which includes a function allowing one of the two parties to unilaterally withdraw both parties' funds. I think it's hard to argue that such an escrow contract would be a suitable implementation and so it should follow for this contract too.

The contest readme also explicitly asks for rug vectors of which this is a clear example.

Recommended Mitigation Steps

Ideally implement the contract without the use of a proxy. As the sale is intended as a one off there's little need for upgradability and this will allow many gas savings as well as avoiding rug potential.

If it is still desired to use a proxy pattern then TokenSaleUpgradeable should be modified to atomically send out CTDL to buyers on the buy call to avoid holding both a buyer's funds and their bought CTDL simultaneously.

Lines of code

https://github.com/code-423n4/2022-02-badger-citadel/blob/84596551d62f243d13fcb2d486346dde08002f7b/contracts/TokenSaleUpgradeable.sol#L144-L148 https://github.com/code-423n4/2022-02-badger-citadel/blob/84596551d62f243d13fcb2d486346dde08002f7b/contracts/TokenSaleUpgradeable.sol#L216-L224 https://github.com/code-423n4/2022-02-badger-citadel/blob/84596551d62f243d13fcb2d486346dde08002f7b/contracts/TokenSaleUpgradeable.sol#L300-L309

Vulnerability details

Impact

Buyer's cannot set a minimum amount of CTDL that they will expect so BadgerDAO can force them to receive a smaller amount than expected.

Proof of Concept

The buy function only allows the buyer to specify an amount of tokenIn to send to the contract and will accept any amount of tokenOut that they receive in return. Simultaneously, BadgerDAO may set tokenOutPrice at any point in the sale to arbitrary values.

1. Buyer broadcasts buy transaction sending 10 WBTC expecting to receive 10 CTDL in return
2. BadgerDAO frontruns this transaction to call setTokenOutPrice to set tokenOutPrice = max_uint256
3. Buyer's transaction confirms and they receive ~0 CTDL in return for their WBTC.

As both the seller and the admin of the contract, BadgerDAO is both incentivised and able to manipulate tokenOutPrice throughout the sale in order to maximise the amount of WBTC they receive.

See similar issue: https://github.com/code-423n4/2021-05-nftx-findings/issues/51

Recommended Mitigation Steps

1. Prevent modifications to the token exchange rate after the sale has started.
2. Allow buyers to pass a minimum value of CTDL that they would expect.

List of contents

• Usage of proxy pattern gives up many gas savings for little benefit
• tokenOut.decimals() may be cached to save external calls
• Excess reads from storage in buy function
• Storage variables may be packed

Usage of proxy pattern gives up many gas savings for little benefit

TokenSaleUpgradeable is expected to be used to perform a one-off sale for a short period of time after which it will no longer be used. It's then very unlikely that the implementation should legitimately need to be upgraded and so a proxy pattern is unnecessary.

If this was not deployed through a proxy we'd save both the costs of interaction with the proxy contract as well as allow the use of gas-saving measures such as immutable variables. Considering the number of storage reads performed as well as the fact that many of these values likely do not need to be updated after deployment this would be a significant gas saving.

tokenOut.decimals() may be cached to save external calls

getAmountOut makes a tokenOut.decimals() call each time it is called. As the decimals value for the CTDL token will not change over time the value 10**tokenOut.decimals can be cached in this contract to avoid the costs of an external call.

https://github.com/code-423n4/2022-02-badger-citadel/blob/84596551d62f243d13fcb2d486346dde08002f7b/contracts/TokenSaleUpgradeable.sol#L216-L224

Alternatively, this value could be rolled into tokenOutPrice to avoid having to perform either the call or an SLOAD.

Excess reads from storage in buy function

Here we read saleStart from storage rather than caching in memory resulting in an extra SLOAD

https://github.com/code-423n4/2022-02-badger-citadel/blob/84596551d62f243d13fcb2d486346dde08002f7b/contracts/TokenSaleUpgradeable.sol#L149-L153

Storage variables may be packed

Many of the variables use a full uint256 slot whereas their maximum value will almost certainly fit in a much smaller value. The contract owner can control the maximum possible value for the linked variables and so can safely use smaller types so that they may be packed into storage. e.g. if tokenInLimit is set to fit in a uint128 then it can clearly be packed with totalTokenIn.

https://github.com/code-423n4/2022-02-badger-citadel/blob/84596551d62f243d13fcb2d486346dde08002f7b/contracts/TokenSaleUpgradeable.sol#L31-L33 https://github.com/code-423n4/2022-02-badger-citadel/blob/84596551d62f243d13fcb2d486346dde08002f7b/contracts/TokenSaleUpgradeable.sol#L42-L50