Badger Citadel contest - cmichel's results

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L888

Vulnerability details

Impact

The StakedCitadel's deposit function (see _depositFor) mints initial shares equal to the deposited amount. The _mintSharesFor / _withdraw functions also use the balance() function, which includes the contract balance token.balanceOf(address(this)), to compute the shares.

It's possible to increase the share price to very high amounts, such that a future depositor mints zero shares for their deposit. They lose all funds and they can then be stolen by the attacker when they redeem their fair share of the strategy TVL.

POC

Assume there is no whitelist and anyone can deposit. Assume the attacker is the first minter and balance() == 0 and token == USDC. Some victim V wants to deposit 1000.0 USDC = 1e9 USDC. The attacker frontruns the transaction and does the following:

• deposit(amount = 1). This amount is transferred and the attacker receives shares = _amount = 1 share. Then balance() == 1 and totalSuplpy() == 1.
• The attacker now increases contract's token balance by directly transfering 1e9 USDC to the controller such that balance() = token.balanceOf(this) = 1e9 + 1.
• The victim deposit is now mined with their _amount = 1e9. The shares computation is now shares = (_amount * totalSupply()) / _pool = 1e9 * 1 / (1e9 + 1) = 0. They contributed 1e9 USDC to the strategy but receive zero shares.
• The attacker can now call withdraw(1) to burn their single share and receive the entire balance() as they redeem 100% of the share total supply: r = (balance() * _shares) / totalSupply() = balance() * 1 / 1 = 2e9 + 1
• They made 1e9 USDC in profit, the totalSupply() is back to zero and they can repeat this attack for the next depositor.

Recommended Mitigation Steps

The way UniswapV2 prevents this is by requiring a minimum deposit amount and sending 1000 initial shares to the zero address to make this attack more expensive. The same mitigation can be done here. Alternatively, scale the initial amount by a large value like 1e18: if (totalSupply() == 0) shares = _amount * 1e18.

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/KnightingRound.sol#L336

Vulnerability details

Impact

The tokenOutPrice can be changed at any time, even while the sale is still running. Users who want to buy a certain amount of tokenOut using buy can (even accidentally) be frontrun by a setTokenOutPrice function call, which reduces the tokenOut amounts they receive. It could be that they didn't want to buy the tokens at the new price.

Recommended Mitigation Steps

Only allow changes to the token price if the sale has not started or add a min out amount parameter to the buy function and revert if the amountOut is less than this parameter.

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadelVester.sol#L143

Vulnerability details

Impact

When withdrawing in StakedCitadel, the vesting is reset in StakedCitadel.setupVest. (There was a bad commit, the vest function is supposed to be called setupVesting.)

Users that withdrew shares a month ago and did not claim will have their entire vesting reset again. Even if they claimed, the remaining vesting balance will now vest according to the new terms which is a worse vesting schedule than before. Users don't receive their tokens according to the vesting schedule, breaking core functionality of the vesting contract.

Furthermore, the note is incorrect:

note that a given address can only have one active vest at a time.

Recommended Mitigation Steps

You can't get correct vesting by having a single vesting schedule for an account and overwriting the single existing vesting schedule all the time. Each vest should be non-fungible as it has its own (recipient, amount, vestStart, vestEnd) parameters.

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L822

Vulnerability details

Impact

The withdraw function burns _shares amounts and the caller is supposed to receive their fair share of r = (balance() * _shares) / totalSupply() tokens. However, this r value can be reduced if not enough tokens are in the contract and the strategy's withdrawal return fewer tokens.

if (_diff < _toWithdraw) {
    // @audit burns more shares than the fair share for b + _diff
    r = b + _diff;
}

When this code is executed, the user receives fewer tokens than the _shares amount they burned. The received amount is not fair anymore.

Recommended Mitigation Steps

If the strategy can currently not withdraw the entire requires _toWithdraw amount, should the _shares to burn be readjusted to match r = b + _diff?

// pseudo-code: burning of _shares needs to happen later
if (_diff < _toWithdraw) {
    r = b + _diff;
+   // @audit important to round up here so user's can't abuse
+   _shares = r * totalSupply() / balance() + 1;
}