prePO contest - 0xkatana's results

Duplicate ReentrancyGuard imports

Impact

The PrePOMarketFactory.sol contract imports 3 ReentrancyGuard files, and one of these 3 files is ReentrancyGuardUpgradeable.sol instead of ReentrancyGuard.sol. This situation can lead to confusion whether the contract is using the ReentrancyGuardUpgradeable.sol import or the ReentrancyGuard.sol import.

Proof of Concept

import   "./LongShortToken.sol";            // imports ReentrancyGuard.sol
import   "./PrePOMarket.sol";               // imports ReentrancyGuard.sol
import   "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";

Tools Used

Manual analysis

Recommended Mitigation Steps

Remove unnecessary ReentrancyGuard imports to avoid confusion whether ReentrancyGuardUpgradeable is used or not.

Unused Import

Impact

Importing ReentrancyGuard.sol is unnecessary in LongShortToken.sol because the import is never used

Proof of Concept

https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/LongShortToken.sol

Tools Used

Manual analysis

Recommended Mitigation Steps

Remove this line from LongShortToken import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

Save gas with unchecked

Impact

If arithmetic will not overflow, use unchecked to save gas. There are several locations where safeMath is not needed and unchecked can lower gas consumption.

Proof of Concept

In this code from Collateral.sol, _balanceAfter ≥ _balanceBefore so unchecked can be used

uint256 _balanceBefore = _baseToken.balanceOf(address(this));
_strategyController.withdraw(address(this), _owed);
uint256 _balanceAfter = _baseToken.balanceOf(address(this));

uint256 _amountWithdrawn = _balanceAfter - _balanceBefore;

Instead, replace the last line with

unchecked { uint256 _amountWithdrawn = _balanceAfter - _balanceBefore; }

Tools Used

Manual analysis

Recommended Mitigation Steps

Use unchecked for arithmetic gas optimizations

Redundant zero initialization

Impact

Solidity does not recognize null as a value, so uint variables are initialized to zero. Setting a uint variable to zero is redundant and can waste gas.

Proof of Concept

https://github.com/code-423n4/2022-03-prepo/blob/main/contracts/core/Collateral.sol#L81

uint256 _shares = 0;

Tools Used

Manual analysis

Recommended Mitigation Steps

Remove the redundant zero initialization uint256 _shares;