prePO contest - cmichel's results

Lines of code

https://github.com/code-423n4/2022-03-prepo/blob/f63584133a0329781609e3f14c3004c1ca293e71/contracts/core/Collateral.sol#L89 https://github.com/code-423n4/2022-03-prepo/blob/f63584133a0329781609e3f14c3004c1ca293e71/contracts/core/SingleStrategyController.sol#L80

Vulnerability details

Impact

The Collateral.deposit function mints initial shares equal to the deposited amount. The deposit / withdraw functions also use the _strategyController.totalValue(), which includes the strategy contract balance, to compute the shares.

It's possible to increase the share price to very high amounts, such that a depositors 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 the attacker is the first minter and collateral.totalSupply() == 0 and _baseToken == USDC. Some victim V wants to deposit 1000.0 USDC = 1e9 USDC. The attacker frontruns the transaction and does the following:

• deposit(amount = 2). Then _fee = 1 and final _amountToDeposit = 2 - 1 = 1. This amount is deposited into the strategy controller and the attacker receives _shares = _amountToDeposit = 1 share. Assume the 1 token is deposited into the strategy and _strategy.totalValue() = 1
• The attacker now increases the strategy controller's balance by directly transfering 1e9 USDC to the controller such that _baseToken.balanceOf(_strategyController) = 1e9 and _strategyController.totalValue() equals _baseToken.balanceOf(address(this)) + _strategy.totalValue() = 1e9 + 1
• The victim deposit is now mined and (even assuming no fees) their _amountToDeposit = 1e9. The shares computation is now _shares = (_amountToDeposit * totalSupply()) / (_valueBefore) = 1e9 * 1 / (1e9 + 1) = 0. They contributed 1e9 USDC to the strategy but receives zero shares.
• The attacker can now call withdraw(1) to burn their single share and receive the entire strategy TVL as they redeem 100% of the total supply: _owed = (_strategyController.totalValue() * _amount) / totalSupply() = _strategyController.totalValue() * 1) / 1 = _strategyController.totalValue() = 2e9 + 1
• They made 1e9 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 = _amountToDeposit * 1e18.

Lines of code

https://github.com/code-423n4/2022-03-prepo/blob/f63584133a0329781609e3f14c3004c1ca293e71/contracts/core/Collateral.sol#L97

Vulnerability details

Impact

After initiating a withdrawal with initiateWithdrawal, it's still possible to transfer the collateral tokens. This can be used to create a second account, transfer the accounts to them and initiate withdrawals at a different time frame such that one of the accounts is always in a valid withdrawal window, no matter what time it is. If the token owner now wants to withdraw they just transfer the funds to the account that is currently in a valid withdrawal window.

Also, note that each account can withdraw the specified amount. Creating several accounts and circling & initiating withdrawals with all of them allows withdrawing larger amounts even at the same block as they are purchased in the future.

I consider this high severity because it breaks core functionality of the Collateral token.

POC

For example, assume the _delayedWithdrawalExpiry = 20 blocks. Account A owns 1000 collateral tokens, they create a second account B.

• At block=0, A calls initiateWithdrawal(1000). They send their balance to account B.
• At block=10, B calls initiateWithdrawal(1000). They send their balance to account A.
• They repeat these steps, alternating the withdrawal initiation every 10 blocks.
• One of the accounts is always in a valid withdrawal window (initiationBlock < block && block <= initiationBlock + 20). They can withdraw their funds at any time.

Recommended Mitigation Steps

If there's a withdrawal request for the token owner (_accountToWithdrawalRequest[owner].blockNumber > 0), disable their transfers for the time.

// pseudo-code not tested
beforeTransfer(from, to, amount) {
  super();
  uint256 withdrawalStart =  _accountToWithdrawalRequest[from].blockNumber;
  if(withdrawalStart > 0 && withdrawalStart + _delayedWithdrawalExpiry < block.number) {
    revert(); // still in withdrawal window
  }
}

Lines of code

https://github.com/code-423n4/2022-03-prepo/blob/f63584133a0329781609e3f14c3004c1ca293e71/contracts/core/Collateral.sol#L328

Vulnerability details

Impact

The getSharesForAmount function returns 0 if totalAssets == 0.

However, if totalSupply == 0, the actual shares that are minted in a deposit are _amount even if totalAssets == 0.

Contracts / frontends that use this function to estimate their deposit when totalSupply == 0 will return a wrong value.

Recommended Mitigation Steps

function getSharesForAmount(uint256 _amount)
    external
    view
    override
    returns (uint256)
{
+   // to match the code in `deposit`
+   if (totalSupply() == 0) return _amount;

    uint256 _totalAssets = totalAssets();
    return
        (_totalAssets > 0)
            ? ((_amount * totalSupply()) / _totalAssets)
            : 0; // @audit this should be _amount according to `deposit`
}

QA

• allowAccounts emits an AccountAllowed even if account is already allowed
• Gas: DepositHook/WithdrawHook.hook never uses _initialAmount variable. Can be removed to save gas on calldata.
• Gas: _floor/_ceilingValuation are never used and never updated. can be removed from storage to save gas, should just read from MarketCreated event as they are never updated.