Tracer contest - cmichel's results

Handle

cmichel

Vulnerability details

The Liquidation contract allows the liquidator to submit "bad" trade orders and the insurance reimburses them from the insurance fund, see Liquidation.claimReceipt. The function can be called with an orders array which does not check for duplicate orders. An attacker can abuse this to make a profit by liquidating themselves, making a small bad trade and repeatedly submitting this bad trade for slippage reimbursement.

Example:

• Attacker uses two accounts, one as the liquidator and one as the liquidatee.
• They run some high-leverage trades such that the liquidatee gets liquidated with the next price update. (If not cash out and make a profit this way through trading, and try again.)
• Liquidator liquidates liquidatee
• They now do two trades:
  • One "good" trade at the market price that fills 99% of the liquidation amount. The slippage protection should not kick in for this trade
  • One "bad" trade at a horrible market price that fills only 1% of the liquidation amount. This way the slippage protection kicks in for this trade
• The liquidator now calls claimReceipt(orders) where orders is an array that contains many duplicates of the "bad" trade, for example 100 times. The calcUnitsSold function will return unitsSold = receipt.amountLiquidated and a bad avgPrice. They are now reimbursed the price difference on the full liquidation amount (instead of only on 1% of it) making an overall profit

This can be repeated until the insurance fund is drained.

Impact

The attacker has an incentive to do this attack as it's profitable and the insurance fund will be completely drained.

Recommended Mitigation Steps

Disallow duplicate orders in the orders argument of claimReceipt. This should make the attack at least unprofitable, but it could still be a griefing attack. A quick way to ensure that orders does not contain duplicates is by having liquidators submit the orders in a sorted way (by order ID) and then checking in the calcUnitsSold for loop that the current order ID is strictly greater than the previous one.

Handle

cmichel

Vulnerability details

The GasOracle uses two chainlink oracles (GAS in ETH with some decimals, USD per ETH with some decimals) and multiplies their raw return values to get the gas price in USD.

However, the scaling depends on the underlying decimals of the two oracles and could be anything. But the code assumes it's in 18 decimals.

"Returned value is USD/Gas * 10^18 for compatibility with rest of calculations"

There is a toWad function that seems to involve scaling but it is never used.

Impact**

If the scale is wrong, the gas price can be heavily inflated or under-reported.

Recommended Mitigation Steps

Check chainlink.decimals() to know the decimals of the oracle answers and scale the answers to 18 decimals such that no matter the decimals of the underlying oracles, the latestAnswer function always returns the answer in 18 decimals.

Handle

cmichel

Vulnerability details

The Chainlink API (latestAnswer) used in the GasOracle oracle wrappers is deprecated:

This API is deprecated. Please see API Reference for the latest Price Feed API. Chainlink Docs

Impact**

It seems like the old API can return stale data. Checks similar to that of the new API using latestTimestamp and latestRoundare are needed. This could lead to stale prices according to the Chainlink documentation:

• under current notifications: "if answeredInRound < roundId could indicate stale data."
• under historical price data: "A timestamp with zero value means the round is not complete and should not be used."

Recommended Mitigation Steps

Add the recommended checks:

(
    uint80 roundID,
    int256 price,
    ,
    uint256 timeStamp,
    uint80 answeredInRound
) = chainlink.latestRoundData();
require(
    timeStamp != 0,
    “ChainlinkOracle::getLatestAnswer: round is not complete”
);
require(
    answeredInRound >= roundID,
    “ChainlinkOracle::getLatestAnswer: stale data”
);
require(price != 0, "Chainlink Malfunction”);

Handle

cmichel

Vulnerability details

The ERC20.transfer() and ERC20.transferFrom() functions return a boolean value indicating success. This parameter should be checked for success. The Insurance.deposit and Insurace.withdraw functions dp not check the return value:

// deposit
collateralToken.transferFrom(msg.sender, address(this), rawTokenAmount);

// withdraw
collateralToken.transfer(msg.sender, rawTokenAmount);

Impact

Some tokens do not revert if the transfer failed but return false instead. Tokens that don't actually perform the transfer and return false are still counted as a correct transfer.

Recommended Mitigation Steps

We recommend using OpenZeppelin’s SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handle the return value check as well as non-standard-compliant tokens.

Handle

cmichel

Vulnerability details

The TracerPerpetualSwaps.settle function updates the user's last index to currentGlobalFundingIndex, however a comment states:

"// Note: global rates reference the last fully established rate (hence the -1), and not the current global rate. User rates reference the last saved user rate"

The code for the else branch also updates the last index to currentGlobalFundingIndex - 1 instead of currentGlobalFundingIndex.

if (accountBalance.position.base == 0) {
    // set to the last fully established index
    // @audit shouldn't this be global - 1 like below?
    accountBalance.lastUpdatedIndex = currentGlobalFundingIndex;
    accountBalance.lastUpdatedGasPrice = IOracle(gasPriceOracle).latestAnswer();
}

Impact

It might be possible that first-time depositors skip having to pay the first funding rate period as the accountLastUpdatedIndex + 1 < currentGlobalFundingIndex check will still return false when the funding rates are updated the next time.

Recommended Mitigation Steps

Check if setting it to currentGlobalFundingIndex or to currentGlobalFundingIndex - 1 is correct.

Handle

cmichel

Vulnerability details

The Trader contract accepts two signed orders and tries to match them. Once they are matched and become filled, they can therefore not be matched against other orders anymore.

This allows for a griefing attack where an attacker can deny any other user from trading by observing the mempool and front-running their trades by creating their own order and match it against the counter order instead.

Impact

A trader can be denied from trading. The cost of the griefing attack is that the trader has to match the order themselves, however depending on the liquidity of the order book and the spread, they might be able to do the counter-trade again afterwards, basically just paying the fees.

It could be useful if the attacker is a liquidator and is stopping a user who is close to liquidation from becoming liquid again.

Recommended Mitigation Steps

This seems hard to circumvent in the current design. If the order book is also off-chain, the executeTrade could also be a bot-only function.