RageTrade/Core

Function `ClearingHouse.withdrawProtocolFee()` accepts an arbitrary list of addresses, on which it calls `collectAccruedProtocolFee()` to retrieve amounts to be withdrawn and then subsequently transfers the accumulated amounts of `protocol.settlementToken` from that contract to `teamMultisig()`. As this function is publicly callable and the provided addresses are not checked in any way (i.e. whitelisted) an attacker could deploy a smart contract implementing a function `collectAccruedProtocolFee()` that returns an arbitrary amount and subsequently drain the `ClearingHouse.sol` contract of funds, transferring them to `teamMultisig()`.

The protocol can be paused/unpaused by the governance or the team multisig by calling `ClearingHouse.pause()` and `ClearingHouse.unpause()` respectively. During the pause/unpause operation, the global funding state must be updated for all the pools to prevent funding payments from accumulating while the protocol is paused. However, the list of pools whose global funding state will be updated on pause/unpause must be supplied as an argument `uint32[] calldata allPoolIds` to each of these functions. This introduces a manual error risk in which some pools' global funding state is not updated on the pause/unpause operation.

Liquidation related mechanisms in the protocol can have an additional fee called `fixFee` that are paid to keepers, in addition to the keeper fee. However, the internal functions that handle the liquidation mechanisms (`ClearingHouse._removeLimitOrder()`, `ClearingHouse._liquidateLiquidityPositions()`, and `ClearingHouse._liquidateTokenPosition()`) all pass `0` as the `fixFee` argument, instead of referencing the `_getFixFee()` function (this function currently returns zero but it can be overriden in specific chain implementations).

When users call liquidation related functions in `ClearingHouse.multicallWithSingleMarginCheck()`, the `accountId` variable is always passed as the liquidation target. However, `ClearingHouse.multicallWithSingleMarginCheck()` always enforces the check `ClearingHouse._getAccountAndCheckOwner()` to ensure that `msg.sender` and the variable `accountId` are the same, meaning that the keeper cannot liquidate another account.

The `VToken` contract has a function `VToken.setVPoolWrapper()` that is used to authorize the `VPoolWrapper.sol` contract so that only it can mint the virtual token. However, this is an external function without permission check, so there is a possibility of the function getting called by another entity after deployment. As of now, this is not an issue since `VToken.setVPoolWrapper()` is called in the same function as the `VToken` deployment (in `RageTradeFactory.initializePool()`). Nevertheless, it is a good security practice to add permission checks on important functions like this.

The governance or team multisig privileges can be transferred to another address simply by calling `Governable.transferGovernance()` or `Governanble.transferTeamMultisig()` functions respectively. These two functions immediately transfer a high level of privileges to new addresses in a single transaction, which can be risky from a security perspective. <br/><br/> A more common pattern for governance or team multisig privilege transfers is to require the new pending addresses to issue an `acceptAdmin()` function call before finalizing the transfer. Note that this pattern is common even with the use of timelocks, such as the [Compound Timelock](https://github.com/compound-finance/compound-protocol/blob/master/contracts/Timelock.sol) contract.

The funding rate calculation in `FundingPayment.getFundingRate()` has an inverted sign because the funding rate is computed with the numerator `int256(realPriceX128) - int256(virtualPriceX128)`, as opposed to the usual formula `int256(virtualPriceX128) - int256(realPriceX128)`. While this inverted sign is corrected by further inverting in `LiquiditionPosition.unrealizedFundingPayment()` and `VTokenPosition.unrealizeFundingPaymment()`, this could lead to other errors in funding rate calculation in future development.

There are no checks on whether the values being supplied on collateral, pool, or protocol parameter changes are reasonable or not (`ClearingHouse.updateCollateralSettings()`, `ClearingHouse.updatePoolSettings()`, `ClearingHouse.updateProtocolSettings()`). Given the importance of these parameters, it is important to be able to reject unreasonable values. For example, parameter `_removeLimitOrderFee` in `ClearingHouse.updateProtocolSettings()` is unconstrained, leaving users vulnerable to potentially unexpected very high fees.

`LiquidityPositionSet.longSideRisk` does not stop even when encountering a removed/uninitialized `LiquidityPosition`.

`LiquidityPositionSet.maxNetPosition` does not stop even when encountering a removed/uninitialized `LiquidityPosition`.