prePO contest - UdarTeam's results

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/feat/2022-12-prepo/apps/smart-contracts/core/contracts/PrePOMarket.sol#L65-L74

Vulnerability details

Impact

createMarket function in PrePOMarketFactory.sol contract creates a new PrePOMarket contract. Salt is used for creating the contract which is computed from _createPairTokens function. Variables passed to this function are visible from anyone (they are input parameter for the createMarket function). After the PrePOMarket contract is created, only the team should mint Long and Short Tokens (Reference). This check is done with hook function from MintHook.sol contract. When the contract is created, _mintHook variable points to zero address, which means that the check here will not execute. Since all transactions are visible in the mempool for a short while before being executed, there could be an observer which can see the PrePOMarket contract creation, compute the contract's address and call mint function before the team has set the mintHook's address. This can break the team's market economy or other things which depend on Long/Short Tokens.

Proof of Concept

Tools Used

Manual

Recommended Mitigation Steps

One way is to set the mintHook variable in PrePOMarket.sol constructor when creating the contract in createMarket function. The other way is to use some type of enhanced commit and reveal schemes.

[N-01] LARGE MULTIPLES OF TEN SHOULD USE SCIENTIFIC NOTATION

Use (e.g. 1e6) rather than decimal literals (e.g. 1000000), for better code readability

apps/smart-contracts/core/contracts/Collateral.sol: 19 apps/smart-contracts/core/contracts/ManagerWithdrawHook.sol: 12

[N-02] USE SCIENTIFIC NOTATION (E.G. 1E18) RATHER THAN EXPONENTIATION (E.G. 10**18)

Scientific notation should be used for better code readability

apps/smart-contracts/core/contracts/Collateral.sol: 31 apps/smart-contracts/core/contracts/TokenSender.sol: 33

[N-03] MISLEADING COMMENT BLOCK

On lines L8-L9 it says that Users can mint/redeem long/short positions on a specific asset in exchange for Collateral tokens.. On lines L73-L74 there is a comment about the mint which says Minting will only be done by the team, and thus relies on the `_mintHook` to enforce access controls.

[G-01] X = X + Y IS MORE EFFICIENT, THAN X += Y (6 INSTANCES)

packages/prepo-shared-contracts/contracts/NFTScoreRequirement.sol: 60 apps/smart-contracts/core/contracts/DepositRecord.sol: 31, 32, 36 apps/smart-contracts/core/contracts/WithdrawHook.sol: 64, 71

[G-02] UNNECESSARY STORAGE READ ON EVENT EMITTING (1 INSTANCE)

apps/smart-contracts/core/contracts/DepositRecord.sol: 42

   function setGlobalNetDepositCap(uint256 _newGlobalNetDepositCap) external override onlyRole(SET_GLOBAL_NET_DEPOSIT_CAP_ROLE) {
     globalNetDepositCap = _newGlobalNetDepositCap;
-    emit GlobalNetDepositCapChange(globalNetDepositCap);
+    emit GlobalNetDepositCapChange(_newGlobalNetDepositCap);
   }