prePO contest - shark's results

1. Zero address/value checks should be implemented in the constructor()

Zero address/value checks should be implemented in the constructor() to mitigate any human errors. Not doing so could lead to redeploying the whole contract.

For example:

File: Collateral.sol Line 29-32

  constructor(IERC20 _newBaseToken, uint256 _newBaseTokenDecimals) {
    baseToken = _newBaseToken;
    baseTokenDenominator = 10**_newBaseTokenDecimals;
  }

As you can see, no zero address/value checks are made. Moreover, baseToken and baseTokenDenominator are both immutable, meaning they cannot be reassigned.

2. Lack of indexed events fields

Indexed parameters are treated as log topics instead of data, making them more easily accessible to off-chain tools. Up to three event parameters can have the indexed keyword. Note that indexing event parameters cost more gas.

Here are some instances of this issue:

File: ITokenSender.sol Line 23

File: IAllowedMsgSenders.sol Line 19

File: INFTScoreRequirement.sol line 17

File: IPrePOMarketFactory.sol Line 14

3. Use of an OpenZeppelin draft contract

The following instance imports an OpenZeppelin draft contract that isn't ready for mainnet use. Due to the nature of drafts, they may change and are not guaranteed stability. Using a draft contract in production can cause the permit function to fail unexpectedly and impact the protocol and its users.

File: Collateral.sol Line 5

import "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/draft-ERC20PermitUpgradeable.sol";

4. Unresolved TODOs

Consider resolving open TODOs before deployment.

Instances found:

File: TokenSender.sol Line 15

File: IDepositRecord.sol Line 4

File: ERC20Mintable.sol Line 7

5. Limit line length

To comply with the Solidity Style Guide, lines should be limited to 120 characters max.

The instances listed below are over 120 characters long:

File: PrePOMarketFactory.sol Line 22, Line 28, Line 41 File: DepositTradeHelper.sol Line 22, Line 24, Line 32 File: ManagerWithdrawHook.sol Line 17, Line 41 File: MintHook.sol Line 16 File: PrePOMarket.sol Line 42 File: DepositHook.sol Line 73

6. Typo/spelling mistakes

File: ICollateral.sol Line 48, Line 95, Line 100

withdraw -> withdrawal

48:   * @param fee The new factor for calculating withdraw fees
...
95:   * expanded functionality such as pausability and withdraw limits.
...
100:   * Does not allow withdraw amounts small enough to result in

File: ICollateral.sol Line 166

  /// @return The factor used to calculate fees for depositinng

depositinng -> depositing

File: MintHook.sol Line 12

   * @dev Since, minting of PrePOMarket positions will only be done by

Consider removing the unnecessary comma: Since, -> Since

7. Events are missing old value

Consider emitting both the old and new values for events that change a state variable.

Here are some instances of this issue:

File: ICollateral.sol Line 38, Line 44, Line 50, Line 56, Line 62, Line 68

38:  event ManagerChange(address manager);
...
44:  event DepositFeeChange(uint256 fee);
..
50:  event WithdrawFeeChange(uint256 fee);
...
56:  event DepositHookChange(address hook);
...
62:  event WithdrawHookChange(address hook);
...
68:  event ManagerWithdrawHookChange(address hook);

File: ITokenSenderCaller.sol Line 17

  event TreasuryChange(address treasury);

8. Unused imports

Contracts that are imported and not used anywhere in the code are most likely an accident due to incomplete refactoring. Such imports can lead to confusion by readers.

Here are some of the instances:

File: PrePOMarketFactory.sol (Line 4, Line 6)

4: import "./LongShortToken.sol";

6: import "./interfaces/ILongShortToken.sol";