Platform: Code4rena
Start Date: 24/06/2021
Pot Size: $80,000 USDC
Total HM: 18
Participants: 12
Period: 7 days
Judge: cemozer
Total Solo HM: 11
Id: 16
League: ETH
Rank: 5/12
Findings: 5
Award: $7,250.47
🌟 Selected for report: 6
🚀 Solo Findings: 0
shw
An attacker can artificially move a market's hourly average price (i.e., the result of getHourlyAvgTracerPrice) by executing a large number of trades on the market with only paying gas fees.
The hourly average price is calculated by the cumulativePrice divided by the number of trades in the given hour (i.e., the average of all trade prices). Therefore, an attacker can bias this average price by executing a large number of trades (i.e., calling executeTrade on Trader with many matched orders), and all of them have an extremely high (or low) trade price, as long as the maker and taker's positions are valid after the trade.
The attacker only pays the gas fees without losing the assets since the makers and takers are all his accounts. Besides, the attacker can avoid paying the trade fees if the fillAmount of trade is 0.
Referenced code: Trader.sol#L121-L126 TracerPerpetualSwaps.sol#L280 Pricing.sol#L100 Pricing.sol#L126-L129 Pricing.sol#L254-L256 LibPrices.sol#L41-L49
This attack is generally difficult to prevent since anyone can execute trades and match orders generated by him. A possible mitigation is to modify the hourly average price formula: increase the cumulativePrice by the trade price multiply the fill amount of each trade. As a result, the attacker has to increase the trade volume to move the average price effectively, and thus the charged trade fees are increased for launching such attacks.
#0 - raymogg
2021-07-05T02:58:32Z
Duplicate of issue #119. Worded slightly differently but it comes down to the fact that
366.8789 USDC - $366.88
shw
According to Chainlink's documentation, the latestAnswer function is deprecated. This function does not error if no answer has been reached but returns 0. Besides, the latestAnswer is reported with 18 decimals for crypto quotes but 8 decimals for FX quotes (See Chainlink FAQ for more details). A best practice is to get the decimals from the oracles instead of hard-coding them in the contract.
Referenced code: ChainlinkOracleAdapter.sol#L30 GasOracle.sol#L32-L33
Referenced documentation: Chainlink - Deprecated API Reference Chainlink - FAQ Chainlink - Migration Instructions Chainlink - API Reference
Use the latestRoundData function to get the price instead. Add checks on the return data with proper revert messages if the price is stale or the round is uncomplete, for example:
(uint80 roundID, int256 price, , uint256 timeStamp, uint80 answeredInRound) = oracle.latestRoundData(); require(answeredInRound >= roundID, "..."); require(timeStamp != 0, "...");
#0 - loudoguno
2021-08-23T22:47:54Z
closing as duplicate of #73
shw
In some contracts (e.g., TracerPerpetualSwaps.sol), the return values of ERC20 transfer and transferFrom are not checked to be true, which could be false if the transferred tokens are not ERC20-compliant. In that case, the transfer fails without being noticed by the calling contract.
Referenced code:
Code using transfer:
TracerPerpetualSwaps.sol#L203
TracerPerpetualSwaps.sol#L514
Insurance.sol#L97
SafetyWithdraw.sol#L13
Code using transferFrom:
TracerPerpetualSwaps.sol#L151
Insurance.sol#L51
Use the SafeERC20 library implementation from Openzeppelin and call safeTransfer or safeTransferFrom when transferring ERC20 tokens.
#0 - raymogg
2021-07-05T06:08:50Z
Duplicate of #115
#1 - loudoguno
2021-08-24T16:10:41Z
change risk from 1 to 2 as per judges sheet
905.8738 USDC - $905.87
shw
As written in the to-do comments, reentrancy could happen in the executeTrade function of Trader since the makeOrder.market can be a user-controlled external contract.
Referenced code: Trader.sol#L121-L126
Add a reentrancy guard (e.g., the implementation from OpenZeppelin) to prevent the users from reentering critical functions.
#0 - raymogg
2021-07-06T01:58:47Z
Disputing just as while this is important, its quite explicitly stated in the todo comment and as such is already known by the team as a potential issue.
Realistically shouldn't be too much of a problem with whitelisting of the trader.
#1 - cemozerr
2021-07-18T19:20:34Z
Marking this as medium risk as, regardless of being noted by the team, still poses a security threat.
#2 - OsmanBran
2021-07-21T02:57:02Z
Duplicate of #72
#3 - loudoguno
2021-08-24T06:04:21Z
removing duplicate label as per judge
#4 - loudoguno
2021-08-24T16:38:44Z
changing risk from 1 to 2 as per judges sheet
66.0382 USDC - $66.04
shw
The transferOwnership function of Liquidation does not check the provided parameter, newOwner, is non-zero. However, the same function in TracerPerpetualSwaps does. The contract could lose the owner if the parameter is provided as zero accidentally.
Referenced code: Liquidation.sol#L445-L447
Add a require(newOwner != address(0), "...") check after line 445.
#0 - OsmanBran
2021-08-03T06:12:22Z
#1 - loudoguno
2021-08-17T23:29:20Z
closing to reflect findings from judges sheet as duplicate of #49
301.9579 USDC - $301.96
shw
Most of the contracts use an unlocked pragma (e.g., pragma solidity ^0.8.0) which is not fixed to a specific Solidity version. Locking the pragma helps ensure that contracts do not accidentally get deployed using a different compiler version with which they have been tested the most.
Referenced code:
Please use grep -R pragma . to find the unlocked pragma statements.
Lock pragmas to a specific Solidity version. Consider the compiler bugs in the following lists and ensure the contracts are not affected by them. It is also recommended to use the latest version of Solidity when deploying contracts (see Solidity docs).
Solidity compiler bugs: Solidity repo - known bugs Solidity repo - bugs by version
#0 - raymogg
2021-07-08T02:49:27Z
Disagree with severity as the Solidity version is defined in the project config as well so the risk of the contracts being deployed with the wrong version is low. Should be a 0
#1 - cemozerr
2021-07-18T18:03:21Z
Marking this as low risk as unlocked pragma can lead to compiler bugs.
shw
The chainId information included in the EIP712_DOMAIN of Tracer is hard-coded and could not change after the contract is deployed. However, if a hard fork happens afterward, the domainSeperator would become invalid on one of the forked chains due to the change of chain ID. Besides, the chain ID of the Ethereum Mainnet should be 1 instead of 1337.
Referenced code: Trader.sol#L28 Trader.sol#L42-L50
Get the current chain ID from block.chainid. Consider using the implementation from OpenZeppelin, which recalculates the domain separator after a hard fork happens.
#0 - raymogg
2021-07-05T06:08:21Z
Duplicate of #67
301.9579 USDC - $301.96
shw
The averagePriceForPeriod function of LibPrices does not handle the case where j equals 0 (i.e., no trades happened in the last 24 hours). The transaction reverts due to dividing by 0 without a proper error message returned.
Referenced code: LibPrices.sol#L73
Add require(j > 0, "...") before line 73 to handle this special case.
🌟 Selected for report: shw
671.0176 USDC - $671.02
shw
The getHourlyAvgTracerPrice and getHourlyAvgOraclePrice functions in Pricing return 0 if there is no trade during the given hour because of the design of averagePrice, which could mislead users that the hourly average price is 0. The same problem happens when emitting the old hourly average in the recordTrade function.
Referenced code: Pricing.sol#L254-L256 Pricing.sol#L262-L264 Pricing.sol#L74
Return a special value (e.g., type(uint256).max) from averagePrice if there is no trade during the specified hour to distinguish from an actual zero price. Handle this particular value whenever the averagePrice function is called by others.
🌟 Selected for report: shw
671.0176 USDC - $671.02
shw
The leveragedNotionalValue function of LibBalance gets the margin value of a position (i.e., the marginValue variable) to calculate the notional value. However, the position's margin value is not checked to be non-negative. Margin with a value less than zero is considered invalid and should be specially handled.
Referenced code: LibBalances.sol#L80
Check whether marginValue is less than zero and handle this case.
#0 - OsmanBran
2021-07-19T03:07:19Z
Although in a normal state marginValue should not be negative (due to being liquidated prior to this), this function should still handle negative values for marginValue and result in valid calculations. Reverting the function due to negative margin values will cause undesirable side-effects in the system.
🌟 Selected for report: shw
671.0176 USDC - $671.02
shw
The recordTrade function in Pricing updates the currentHour variable by 1 every hour. However, if there is no trade (i.e., the recordTrade is not called) during this hour, the currentHour is out of sync with the actual hour. As a result, the averagePriceForPeriod function uses the prices before 24 hours and causes errors on the average price.
Referenced code: Pricing.sol#L90-L94
Calculate how much time passed (e.g., (block.timestamp - startLastHour) / 3600) to update the currentHour variable correctly.