Tracer contest - gpersoon's results

Handle

gpersoon

Vulnerability details

Impact

The function transferOwnership of TracerPerpetualSwaps.sol contains a check for address(0), however the function transferOwnership of Liquidation.sol does not contain this check. So the ownership could be made inaccessible by accident.

Proof of Concept

//https://github.com/code-423n4/2021-06-tracer/blob/main/src/contracts/TracerPerpetualSwaps.sol#L572 function transferOwnership(address newOwner) public override(Ownable, ITracerPerpetualSwaps) onlyOwner { require(newOwner != address(0), "address(0) given"); super.transferOwnership(newOwner); }

//https://github.com/code-423n4/2021-06-tracer/blob/main/src/contracts/Liquidation.sol#L445 function transferOwnership(address newOwner) public override(Ownable, ILiquidation) onlyOwner { super.transferOwnership(newOwner); }

Tools Used

Recommended Mitigation Steps

ALso add the following check to transferOwnership of Liquidation.sol: require(newOwner != address(0), "address(0) given");

Handle

gpersoon

Vulnerability details

Impact

The value for LIQUIDATION_GAS_COST is hardcoded in TracerPerpetualSwaps.sol. However with updates in solidity (like eip1559) this gas costs of instructions could change and this value might no longer be accurate.

Proof of Concept

// https://github.com/code-423n4/2021-06-tracer/blob/main/src/contracts/TracerPerpetualSwaps.sol#L26 uint256 public constant override LIQUIDATION_GAS_COST = 63516;

Tools Used

Recommended Mitigation Steps

Make the LIQUIDATION_GAS_COST parameter updatable.

Handle

gpersoon

Vulnerability details

Impact

There are several todos left in the code.

Proof of Concept

.\Pricing.sol: // todo by using public variables lots of these can be removed .\Trader.sol: // todo this could be succeptible to re-entrancy as .\lib\LibLiquidation.sol: // todo with the below * -1, note ints can overflow as 2^-127 is valid but 2^127 is not. .\lib\LibPrices.sol: // todo double check safety of this.

Tools Used

Recommended Mitigation Steps

Check, fix and remove the todos before it is deployed in production

Handle

gpersoon

Vulnerability details

Impact

In function calculateSlippage of LibLiquidation.sol, the value of amountToReturn is calculated by subtracting to numbers. Later on it is check if this value is negative. However amountToReturn is an unsigned integer so it can never be negative. If a negative number would be attempted to be assigned, the code will revert, because solidity 0.8 checks for this.

Proof of Concept

// https://github.com/code-423n4/2021-06-tracer/blob/main/src/contracts/lib/LibLiquidation.sol#L106 function calculateSlippage( ... uint256 amountToReturn = 0; uint256 percentSlippage = 0; if (avgPrice < receipt.price && receipt.liquidationSide == Perpetuals.Side.Long) { amountToReturn = amountExpectedFor - amountSoldFor; } else if (avgPrice > receipt.price && receipt.liquidationSide == Perpetuals.Side.Short) { amountToReturn = amountSoldFor - amountExpectedFor; } if (amountToReturn <= 0) { // can never be smaller than 0, because amountToReturn is uint256 return 0; }

Tools Used

Recommended Mitigation Steps

Double check if amountToReturn could be negative. If this is the case change the type of amountToReturn to int256 and add the appropriate type casts

Handle

gpersoon

Vulnerability details

Impact

The function averagePriceForPeriod calculates the average prices for the last 24 hours. In case no trades have been done, the variable j keeps the value of 0. Then the function meanN is called, which divides by the value of j (e.g. 0). This means the code will revert at that moment and no other code will be executed.

Proof of Concept

// https://github.com/code-423n4/2021-06-tracer/blob/main/src/contracts/lib/LibPrices.sol#L57 function averagePriceForPeriod(PriceInstant[24] memory prices) internal pure returns (uint256) { ... uint256 j = 0; for (uint256 i = 0; i < 24; i++) { .. if (currPrice.trades == 0) { continue; } else { .... j++; } } return LibMath.meanN(averagePrices, j); } // https://github.com/code-423n4/2021-06-tracer/blob/main/src/contracts/lib/LibMath.sol#L67 function meanN(uint256[] memory arr, uint256 len) internal pure returns (uint256) { return sumN(arr, len) / len; }

Tools Used

Recommended Mitigation Steps

Check the error handling for the situation there are no trades for 24 hours.

Handle

gpersoon

Vulnerability details

Impact

If you call the function withdrawFees and the "tvl" would not be enough for the fee then the code would revert. In this case the fees cannot be withdrawn. Although it is unlikely that the tvl would be wrong it is probably better to be able to withdraw the remaining fees.

Proof of Concept

// https://github.com/code-423n4/2021-06-tracer/blob/main/src/contracts/TracerPerpetualSwaps.sol#L508 function withdrawFees() external override { uint256 tempFees = fees; fees = 0; tvl = tvl - tempFees;

    // Withdraw from the account
    IERC20(tracerQuoteToken).transfer(feeReceiver, tempFees);
    emit FeeWithdrawn(feeReceiver, tempFees);
}

Tools Used

Recommended Mitigation Steps

Add something like: tempFees = min (fees, tvl); and change fees=0 to: fees -= tempFees;

Handle

gpersoon

Vulnerability details

Impact

The function matchOrders of TracerPerpetualSwaps.sol doesn't check that the contract itself is indeed equal to order1.market and order2.market. The function executeTrade Trader.sol, which calls the matchOrders, can deal with multiple markets. Suppose there would be a mistake in executeTrade, or in a future version, the matchOrders would be done in the wrong market.

Proof of Concept

// https://github.com/code-423n4/2021-06-tracer/blob/main/src/contracts/TracerPerpetualSwaps.sol#L216 function matchOrders( Perpetuals.Order memory order1, Perpetuals.Order memory order2, uint256 fillAmount )

// https://github.com/code-423n4/2021-06-tracer/blob/main/src/contracts/Trader.sol#L67 function executeTrade(Types.SignedLimitOrder[] memory makers, Types.SignedLimitOrder[] memory takers) external override { ... (bool success, ) = makeOrder.market.call( abi.encodePacked( ITracerPerpetualSwaps(makeOrder.market).matchOrders.selector, abi.encode(makeOrder, takeOrder, fillAmount) ) );

// https://github.com/code-423n4/2021-06-tracer/blob/main/src/contracts/lib/LibPerpetuals.sol#L128 function canMatch( Order memory a, uint256 aFilled,Order memory b, uint256 bFilled ) internal view returns (bool) { ... bool marketsMatch = a.market == b.market;

Tools Used

Recommended Mitigation Steps

Add something like: require ( order1.market == address(this), "Wrong market");

Note: canMatch already verifies that order1.market== order2.market