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: 6/12
Findings: 4
Award: $4,005.09
🌟 Selected for report: 5
🚀 Solo Findings: 0
pauliax
contract TracerPerpetualSwaps is SafetyWithdraw. It allows an owner to transfer all tokens from this contract whenever he wants (e.g. no protection against tracerQuoteToken, etc). Tracer representative's answer on Discord: 'safetyWithdraw isn't needed as a failover mechanism anymore. I'd say thats a valid find'.
Get rid of SafetyWithdraw or protect legitimately deposited user tokens.
#0 - raymogg
2021-07-05T23:09:32Z
Duplicate of #81
#1 - ninek9
2021-08-24T22:15:42Z
changed severity per judge
pauliax
When transfering erc20 tokens, functions transfer and transferFrom are used. These functions return boolean to indicate if the action was sucessfull, however, none of the usages check the returned value: collateralToken.transferFrom(msg.sender, address(this), rawTokenAmount); IERC20(tracerQuoteToken).transferFrom(msg.sender, address(this), rawTokenAmount); collateralToken.transfer(msg.sender, rawTokenAmount); IERC20(tracerQuoteToken).transfer(msg.sender, rawTokenAmount); IERC20(tracerQuoteToken).transfer(feeReceiver, tempFees);
There are lots of possible issues with different erc20 tokens (https://github.com/xwvvvvwx/weird-erc20) but the current best practice to deal with it is using SafeERC20: https://docs.openzeppelin.com/contracts/2.x/api/token/erc20#SafeERC20
#0 - raymogg
2021-07-05T06:43:32Z
Duplicate of #115
#1 - loudoguno
2021-08-24T16:08:17Z
changed risk from 1 to 2 as per judges sheet
pauliax
modifier onlyLiquidation checks that the sender is liquidationContract. However, liquidationContract is not initialized in the constructor and needs to be explicitly set by calling setLiquidationContract. While in practice this will be initialized in the factory contract, I think it still makes sense to have explicit checks that liquidationContract is properly initialized (not 0x0).
feeReceiver in contract TracerPerpetualSwaps also may be initialized to 0x0. While function setFeeReceiver prevents 0x0 address, the first time it is initialized in the constructor where there is no check against empty address thus in theory it is possible that feeReceiver is initialized to an empty address making all the fees being burned (maybe that may be an intended behavior in some cases?). I think it would make sense here to call setFeeReceiver in the constructor as you basically get a check against 0x0 and also an event FeeReceiverUpdated is emitted this way.
Make sure that addresses are properly initialized in all cases (not left empty).
#0 - raymogg
2021-07-08T00:24:34Z
The pattern of setting the liquidation contract is needed due to some circular dependencies.
Marking as a duplicate of #136 as the ticket raises checking zero address
pauliax
Low-level call returns true even if the account called is non-existent. Here it calls a market using low level call: (bool success, ) = makeOrder.market.call( abi.encodePacked( ITracerPerpetualSwaps(makeOrder.market).matchOrders.selector, abi.encode(makeOrder, takeOrder, fillAmount) ) ); // ignore orders that cannot be executed if (!success) continue; If market is not an existing contract, this call will still succeed and will continue the execution. Here is a simple POC that I wrote to demonstrate that: https://gist.github.com/pauliax/54b4bda4f5bfb3d310c169b3427e5b23 Try to execute function lowCall on EOA and see that it succeeds.
This issue was described before by ser 0xRajeev: https://github.com/code-423n4/2021-04-basedloans-findings/issues/16 You can read more about it here (read warning sections): https://docs.soliditylang.org/en/v0.8.6/units-and-global-variables.html?highlight=.call#members-of-address-types
Check against EOA before calling the market or refactor to eliminate low-level call and use the interface.
#0 - raymogg
2021-07-05T06:45:47Z
Duplicate of #126
🌟 Selected for report: pauliax
671.0176 USDC - $671.02
pauliax
Here the check currentMargin < Balances.minimumMargin should be inclusive <= to indicate the account is not above minimum margin: require( currentMargin <= 0 || uint256(currentMargin) < Balances.minimumMargin(pos, price, gasCost, tracer.trueMaxLeverage()), "LIQ: Account above margin" );
uint256(currentMargin) <= Balances.minimumMargin ...
pauliax
Hardcoding chain id is error-prone (in case you will decide to deploy on a different chain and forget to change this or if the chain forks, etc): uint256 public constant override chainId = 1337; // Changes per chain
Better utilize global variable block.chainid or you can also retrieve chain id via assembly: https://ethereum.stackexchange.com/a/77550/17387
#0 - raymogg
2021-07-08T00:24:56Z
Duplicate of #67
#1 - loudoguno
2021-08-24T04:31:28Z
reopening as primary issue for L-18 as per judge
🌟 Selected for report: pauliax
333.3333 USDC - $333.33
pauliax
Trader function executeTrade calculates executionPrice, newMakeAverage, newTakeAverage, then calls the market, and only if it succeeds it uses these variables.
Better first call the market and only then calculate and use these variables to avoid useless calculations and gas costs.
🌟 Selected for report: pauliax
333.3333 USDC - $333.33
pauliax
Insurance function drainPool calculates 10**18 many times. To reduce the number of calculations and save gas, this number can be extracted as a constant variable and used everywhere where necessary.
Extract 10**18 as a constant.
🌟 Selected for report: pauliax
333.3333 USDC - $333.33
pauliax
Could save some gas here when amountToReturn = receipt.escrowedAmount: if (amountToReturn > receipt.escrowedAmount) { liquidationReceipts[receiptId].escrowedAmount = 0; } else { liquidationReceipts[receiptId].escrowedAmount = receipt.escrowedAmount - amountToReturn; }
if (amountToReturn >= receipt.escrowedAmount) { ...