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: 1/12
Findings: 9
Award: $20,073.60
🌟 Selected for report: 14
🚀 Solo Findings: 4
🌟 Selected for report: 0xRajeev
6710.1761 USDC - $6,710.18
0xRajeev
The updateFundingRate() function updates the funding rate and insurance funding rate. While the instant/new funding rates are calculated correctly, the cumulative funding rate calculation is incorrect because it is always adding the instant to 0, not the previous value. This is due to the use of [currentFundingIndex] which has been updated since the previous call to this function while it should really be using [currentFundingIndex-1] to reference the previous funding rate.
Impact: The cumulative funding rate and insurance funding rates are calculated incorrectly without considering the correct previous values. This affects the settling of accounts across the entire protocol. The protocol logic is significantly impacted, accounts will not be settled as expected, protocol shutdown and contracts will need to be redeployed. Users may lose funds and protocol takes a reputation hit.
Manual Analysis
Use [currentFundingIndex-1] for non-zero values of currentFundingIndex to get the value updated in the previous call on lines L155 and L159 of Pricing.sol.
#0 - raymogg
2021-07-05T03:14:20Z
Confirmed as an index issue with funding rate 👍
1811.7475 USDC - $1,811.75
0xRajeev
The withdrawERC20Token() in SafetyWithdraw inherited in TracerPerpetualSwaps is presumably a guarded launch emergency withdrawal mechanism. However, given the trust model where the market creator/owner is potentially untrusted/malicious, this is a dangerous approach to emergency withdrawal in the context of guarded launch.
Alternatively, if this is meant for the owner to withdraw “external” ERC20 tokens mistakenly deposited to the Tracer market then the function should exclude tracerQuoteToken from being the tokenAddress that can be used as a parameter to withdrawERC20Token().
Impact: Malicious owner of a market withdraws/rugs all tracerQuoteTokens deposited at any time after market launch. All users lose deposits. Protocol takes a reputational hit and has to refund the users from treasury.
Manual Analysis
For a guarded launch circuit breaker, design a pause/unpause feature where deposits are paused (in emergency situations) but withdrawals are allowed by the depositors themselves instead of the owner. Alternatively, if this is meant to be for removing external ERC20 tokens accidentally deposited to market, exclude the tracerQuoteToken from being given as the tokenAddress.
#0 - raymogg
2021-07-05T03:13:21Z
The only reason for the dispute on severity is that as part of the security model, the owner can manipulate the market in other ways (such as changing the oracle being used), so this trust assumption over the owner already exists. For this reason the team thinks this issue is closer to a medium
This however is a good issue as it is not the greatest circuit breaking mechanism, and as noted in #7 can reflect badly on the project without the exploit being used. The mechanism is being removed and replaced with more structured circuit breaker.
#1 - cemozerr
2021-07-18T20:04:51Z
Marking this as high risk, as regardless of the owner manipulating in other ways, the threat persists.
0xRajeev
The contracts use Chainlink’s deprecated API latestAnswer(). Such functions might suddenly stop working if Chainlink stopped supporting deprecated APIs.
Impact: Deprecated API stops working. Prices cannot be obtained. Protocol stops and contracts have to be redeployed.
See similar Low-severity finding L11 from OpenZeppelin's Audit of Opyn Gamma Protocol: https://blog.openzeppelin.com/opyn-gamma-protocol-audit/
See https://docs.chain.link/docs/deprecated-aggregatorinterface-api-reference/#latestanswer.
Manual Analysis
Use V3 interface functions: https://docs.chain.link/docs/price-feeds-api-reference/
#0 - raymogg
2021-07-05T09:15:27Z
Duplicate of #145
#1 - cemozerr
2021-07-18T17:47:45Z
Marking this as a medium as it can lead to having the whole protocol being redeployed.
0xRajeev
ERC20 tokens are specified to return a boolean value on token transfer() and transferFrom(). However, tokens may not adhere to the spec and return no value for success/failure. Checking the return values of ERC20 token transfers is therefore important to anticipate all possibilities: 1) return boolean 2) return nothing 3) revert. The SafeERC20 wrappers from OpenZeppelin cover all these scenarios and throw on failure (when the token contract returns false.
Tracer markets allow the market creator to specify any arbitrary ERC20 tracerQuoteToken which may or may not adhere to the ERC20 spec, and may even be malicious. To minimize risk, it will help to use the SafeERC20 wrappers and give better protection to the market users.
Impact: Market creator uses a tracerQuoteToken which returns true/false on success/failure. The code does not check return values of transfers and assumes correct amounts are deposited/withdrawn. This leads to bad accounting and the entire protocol is at risk of insolvency or exploitation.
Manual Analysis
Use SafeERC20 wrapper functions to handle token transfers
#0 - raymogg
2021-07-05T03:25:08Z
Duplicate of #115
905.8738 USDC - $905.87
0xRajeev
The Trader contract makes an external call to the market contract TracerPerpetualSwaps as part of executeTrade(). The comments in code "// todo this could be succeptible to re-entrancy as // market is never verified” indicate that there could be a residual concern for reentrancy. However, it is not clear that this is the case because the factory only deploys TracerPerpetualSwaps market contract which is “trusted” and not expected to make a reentrant call to Trader::executeTrade().
Noting this here as a finding to indicate the concern expressed in the comment for reevaluation. executeTrade() neither has a reentrancy guard nor follows the Checks-Effects-Interactions pattern because order state is updated after the market call to matchOrders().
Manual Analysis
Reevaluate the concern expressed in the comment a a “todo.” Clarify the concern or consider using the OpenZeppelin ReentrancyGuard and/or Checks-Effects-Interactions pattern.
#0 - raymogg
2021-07-08T00:28:50Z
Disputing as the issue is clearly marked in the code. Appreciate it being raised as a concern.
Ideally there will be a check with the factory to ensure that the market address is in fact a deployed tracer market (this fix is already implemented and to be pushed soon).
#1 - cemozerr
2021-07-18T20:58:14Z
Marking this as a valid issue as it still poses a security threat.
#2 - loudoguno
2021-08-24T16:38:18Z
changing risk from 1 to 2 and closing as duplicate of #143 as per judges sheet
🌟 Selected for report: 0xRajeev
2013.0528 USDC - $2,013.05
0xRajeev
The Tracer Perpetuals Factory contract is arguably the most critical contract in the project given that it deploys all the markets. The ownership of this contract is transferred to _governance address, i.e. TracerDAO, in the constructor. This critical address transfer in one-step is very risky because it is irrecoverable from any mistakes.
Impact: If an incorrect address, e.g. for which the private key is not known, is used accidentally then it prevents the use of all the onlyOwner() functions forever, which includes the changing of various deployer contract addresses and market approvals. This use of incorrect address may not even be immediately apparent given that these functions are probably not used immediately. When noticed, due to a failing onlyOwner() function call, it will force the redeployment of the factory contract and require appropriate changes and notifications for switching from the old to new address. This will diminish trust in markets and incur a significant reputational damage.
See similar High Risk severity finding from Trail-of-Bits Audit of Hermez: https://github.com/trailofbits/publications/blob/master/reviews/hermez.pdf
See similar Medium Risk severity finding from Trail-of-Bits Audit of Uniswap V3: https://github.com/Uniswap/uniswap-v3-core/blob/main/audits/tob/audit.pdf
Manual Analysis
Retain the deployer ownership in the constructor and then use a two-step address change to _governance address separately using setter functions: 1) Approve a new address as a pendingOwner 2) A transaction from the pendingOwner (TracerDAO) address claims the pending ownership change. This mitigates risk because if an incorrect address is used in step (1) then it can be fixed by re-approving the correct address. Only after a correct address is used in step (1) can step (2) happen and complete the address/ownership change.
#0 - raymogg
2021-07-05T03:49:12Z
Correct that having the owner be set to a wrong address could be detrimental, however for the first deploy of the factory, this will be owned by the DAO and will be easy to validate on deployment.
Subsequent ownership transfers will be done via DAO proposal, and will have many eyes across them (due to them being a public Tracer DAO proposal) before function execution happens.
For this reason it seems like a lot of overhead to have a two step process for this. Not withstanding that the issue you mention could still be possible
🌟 Selected for report: 0xRajeev
2013.0528 USDC - $2,013.05
0xRajeev
Tracer protocol like any other allows market creators to charge fees for trades. However, a malicious/greedy owner can arbitrarily change fee to any % value and without an event to observe this change or a timelock to react, there is no easy way for users to monitor this via front-end or off-chain monitoring tools.
Impact: Users trade on a market with 0.1% fees. The owner suddenly changes this to 100%. Users realise this only after their trades are executed. Market loses confidence. Protocol takes a reputational hit.
See similar Medium-severity finding in ConsenSys's Audit of 1inch Liquidity Protocol (https://consensys.net/diligence/audits/2020/12/1inch-liquidity-protocol/#unpredictable-behavior-for-users-due-to-admin-front-running-or-general-bad-timing
Manual Analysis
Emit event, provide time lock for users to react and establish an upper threshold for fees that is decided across markets by governance.
#0 - raymogg
2021-07-05T03:42:40Z
Like the idea of having a timelock for any update parameter update that immediately affects traders
🌟 Selected for report: 0xRajeev
0xRajeev
The owner of TracerPerpetualSwaps contract, who is potentially untrusted as per specification, can change the market critical parameters such as the addresses of the Liquidation/Pricing/Insurance/GasOracle/FeeReceiver and also critical values such as feeRate, maxLeverage, fundingRateSensitivity, deleveragingCliff, lowestMaxLeverage, insurancePoolSwitchStage and whitelisting.
None of these setter functions emit events to record these changes on-chain for off-chain monitors/tools/interfaces to register the updates and react if necessary.
Impact: Malicious owner changes the critical addresses or values that significantly change the security posture/perception of the protocol. No events are emitted and users lose funds/confidence. The protocol takes a reputation hit.
See similar High-severity finding OpenZeppelin’s Audit of Audius (https://blog.openzeppelin.com/audius-contracts-audit/#high) and Medium-severity finding OpenZeppelin’s Audit of UMA Phase 4 (https://blog.openzeppelin.com/uma-audit-phase-4/)
Manual Analysis
Consider emitting events when these addresses/values are updated. This will be more transparent and it will make it easier to keep track of the status of the system.
#0 - raymogg
2021-07-05T03:42:06Z
Duplicate of #66
#1 - cemozerr
2021-07-18T19:36:22Z
Opening this issue as the event emission seems to be separate from the arbitrarily changing of the values.
0xRajeev
Zero-address checks are a best-practise for input validation of critical address parameters. While the codebase applies this to most addresses in setters, there are many places where this is missing in constructors and setters.
Impact: Accidental use of zero-addresses may result in exceptions, burn fees/tokens or force redeployment of contracts.
Manual Analysis
Add zero-address checks.
#0 - raymogg
2021-07-05T23:05:24Z
Duplicate of #136
More issues brought up in this one, but falls under the general category of missing zero address checks
#1 - loudoguno
2021-08-17T23:25:56Z
reopening as primary issue to reflect judges findings sheet
181.1748 USDC - $181.17
0xRajeev
The gas cost for liquidation may change if code is updated/optimized, compiler changed or profiling improved. The developers may forget to update this constant in code.
Impact: The margin validity calculation which uses this value may be affected if this changes and hence is not as declared in the constant. This may adversely impact validation.
Manual Analysis
It is safer to make this a constructor-set immutable value that will force usage of an updated accurate value at deployment time. Evaluate if the sensitivity to this value is great enough to justify a setter to change it if incorrectly initialized at deployment.
#0 - raymogg
2021-07-05T23:06:11Z
Duplicate of #101
#1 - loudoguno
2021-08-17T23:35:46Z
reopened as primary issue to reflect judges findings sheet
🌟 Selected for report: 0xRajeev
671.0176 USDC - $671.02
0xRajeev
The Deposit event uses the function parameter amount instead of the convertedWadAmount which is what is used to update the user’s position and tvl because it prevents any dust deposited in amount. This will also make it consistent with the emit event in withdraw function.
Impact: Deposit event amount reflects the value with dust while the user position does not. This may lead to confusion.
Manual Analysis
Use uint256(convertedWadAmount) instead of amount in Deposit event.
🌟 Selected for report: 0xRajeev
671.0176 USDC - $671.02
0xRajeev
The tvl calculation in deposit() uses convertedWadAmount but the one in withdraw() uses the parameter amount. While amount is still in WAD format, it may contain dust which is what the conversion to rawTokenAmount and then back to convertedWadAmount removes.
Impact: Use of amount in tvl during withdraw() will consider dust while the one in deposit() will not, which is inconsistent.
Manual Analysis
Use convertedWadAmount instead of amount to be consistent with the increment during withdraw() tvl calculation.
0xRajeev
Low-level calls call/delegatecall/staticcall return true even if the account called is non-existent (per EVM design). Solidity documentation warns: "The low-level functions call, delegatecall and staticcall return true as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior to calling if needed.”
Market address may not exist as a contract (e.g. incorrect EOA address used in orders) in which case low-level call still returns true/success. But the trade is assumed to have been successfully executed.
Impact: executeTrade() executes batch orders against a non-existing market contract due to a mistake in the trading interface. The transaction executes successfully without any side-effects because the market doesn’t exist. Internal accounting is updated incorrectly.
See related High-severity finding in ToB’s Audit of Hermez: https://github.com/trailofbits/publications/blob/master/reviews/hermez.pdf and ToB’s Audit of Uniswap V3: https://github.com/Uniswap/uniswap-v3-core/blob/main/audits/tob/audit.pdf
See Warning in: https://docs.soliditylang.org/en/v0.8.0/control-structures.html#error-handling-assert-require-revert-and-exceptions
Manual Analysis
makeOrder.market should be checked for contract existence before the low-level call and verified to be actual market contract (but it is not verified as noted in the comment). Evaluate if this is a greater concern than undefined behavior.
#0 - raymogg
2021-07-05T09:22:27Z
Duplicate of #126
#1 - loudoguno
2021-08-24T16:54:28Z
removed duplicate label and reopened as per judges sheet (swapped with #126 )
🌟 Selected for report: 0xRajeev
671.0176 USDC - $671.02
0xRajeev
In verifyAndSubmitLiquidation(), the tx.gasprice is checked against the fastGasOracle’s current gas price presumably to prevent liquidators front-running others for the same market/account by using a gas price exceeding the current prevailing price as indicated by the fastGasOracle.
Impact: If the gas prices are increasing rapidly due to volatility or network congestion, or if the liquidation engines and fastGasOracle are out of sync on gas prices because of consulting different sources, then these liquidations will keep failing. Front-running risk on liquidations is not adequately protected by tx.gasprice check.
This logic may also be impacted by the upcoming inclusion of EIP-1559 in London fork which affect gas semantics significantly.
Manual Analysis
Liquidation bots front-running by monitoring mempool or the use of FlashBots for liquidation MEV is a systemic challenge and not solved by using gasprice logic in contracts. Evaluate if the benefits match the failure modes.
#0 - raymogg
2021-07-08T00:27:26Z
Good point on the strict check of fast gas price. As you pointed out this is to prevent gas auctions from occuring and causing liquidations to be extremely expensive, however this could become a bottleneck in times of rapidly changing gas prices.
Have acknowledged that this could cause problems in certain scenarios. The team will be thinking about a safer implementation of this mechanism.
🌟 Selected for report: 0xRajeev
671.0176 USDC - $671.02
0xRajeev
In function minimumMargin(), maximumLeverage being zero is not handled because it will result in div by zero as PRBMathUD60x18.div expects non-zero divisor.
Impact: Various critical market functions will revert if maximumLeverage is zero.
Manual Analysis
Add checks to make sure maximumLeverage is never zero or handle appropriately.
122.293 USDC - $122.29
0xRajeev
ChainID is hardcoded to 1337 whereas it should be using the deployed chain’s chainID to function correctly. ChainID is used in signatures and verification to prevent replay attacks across hard forks or different chains e.g. L2s. While Trader contract uses ChainID in EIP712_DOMAIN, it is hardcoded to a constant 1337. This is likely a test configuration (not even listed on chainid.network) being carried over to production.
Impact: The current contracts get deployed in production on Ethereum Mainnet whose chainID is 1. Signature verification uses EIP712_DOMAIN which has a chainID of 1337 and signatures fail verification. Execution of trades fail across all markets. Contracts need to be redeployed. Protocol takes a reputation hit.
See related High-severity finding from ToB’s Audit of Yield Protocol: https://github.com/trailofbits/publications/blob/master/reviews/YieldProtocol.pdf
Manual Analysis
Instead of a hardcoded constant, use Solidity’s block.chainID to get currently executing chain’s chainID. Do not use test configuration in production contracts.
#0 - raymogg
2021-07-05T03:17:02Z
Disputing as an issue as while the suggested approach is a better solution (dynamically setting ChainID), deploying a Trader contract with the wrong ID does not affect the system. A new Trader can be deployed using the appropriate ChainID if one is accidentally deployed with the wrong ChainID.
Currently this ChainID is just updated before deployment. The team will implement the dynamic approach moving forward.
#1 - raymogg
2021-07-07T00:02:30Z
If we are considering this an issue, it would probably also be much lower severity, as the ramifications of this are pretty low considering the option to redeploy. Would be a 0 or 1 in my mind.
#2 - cemozerr
2021-07-18T19:08:24Z
Marking this as low risk as there is an option to redeploy.
#3 - loudoguno
2021-08-24T04:32:52Z
closing and labeling as duplicate of #85 as per judge
🌟 Selected for report: 0xRajeev
671.0176 USDC - $671.02
0xRajeev
Trading function executeTrade() batch executes maker/taker orders against a market. The trader/interface provides arrays of makers/takers which is unbounded. As a result, if the number of orders is too many, there is a risk of this transaction exceeding the block gas limit (which is 15 million currently).
Impact: executeTrade() is called with too many orders in the batch. Tx exceeds block gas limit and reverts. None of the orders are executed.
See similar Medium-severity finding from ConsenSys's Audit of Growth DeFi: https://consensys.net/diligence/audits/2020/12/growth-defi-v1/#potential-resource-exhaustion-by-external-calls-performed-within-an-unbounded-loop
Manual Analysis
Limit the number or orders executed based on gasleft() after every iteration or estimate the gas cost and enforce an upper bound on the number of orders allowed in maker/taker arrays.