prePO contest - chaduke's results

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/WithdrawHook.sol#L66-L72

Vulnerability details

Impact

Detailed description of the impact of this finding. The if-else-statement only checks the userWithdrawLimitPerPeriod constraint for the else part, not for the if part. As a result, a user can withdraw any amount in a new period and the contract will fail to detect such violation.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/WithdrawHook.sol#L66-L72 The userWithdrawLimitPerPeriod constraint needs to be checked for the first withdraw as well as for later withdraw in a period. Failing to check for the first withdraw in a period will result a violation of the contraint and as a result, a user can withdraw as much as he wants.

Tools Used

Remix

Recommended Mitigation Steps

A correction is as follows which includes a check in both cases (if case and else case):

 if (lastUserPeriodReset + userPeriodLength < block.timestamp) {
      lastUserPeriodReset = block.timestamp;
      require(_amountBeforeFee <= userWithdrawLimitPerPeriod, "user withdraw limit exceeded");

      userToAmountWithdrawnThisPeriod[_sender] = _amountBeforeFee;
    } else {
      require(userToAmountWithdrawnThisPeriod[_sender] + _amountBeforeFee <= userWithdrawLimitPerPeriod, "user withdraw limit exceeded");
      userToAmountWithdrawnThisPeriod[_sender] += _amountBeforeFee;
    }

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/TokenSender.sol#L36

Vulnerability details

Impact

Detailed description of the impact of this finding. The send() function will fail in three cases: 1) scaledPrice <= _scaledPriceLowerBound; 2) outputAmount == 0; and 3) outputAmount > _outputToken.balanceOf(address(this)). Unfortunately, the function just return so that the caller will not detect the failure and assume everything is fine. For example, the caller will assume the fee has been sent to the recipient successfully even though no fee has been sent

Proof of Concept;

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/TokenSender.sol#L36 The send() function should revert under the three failing conditions so that the caller know the fee has not been sent successful. Or, it can return a boolean value so that the caller can decide what to do when fee has not been sent successfully.

Tools Used

Remix

Recommended Mitigation Steps

Either revert on the failing condition or return false under such condition.

QA1: https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/Collateral.sol#L65 The withdraw function assumes that the collatoral token has 18 decimals, as a result, the withdraw function will not work when the collatoral has different number of decimals.

QA2: https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/Collateral.sol#L45 A common mistake is that user might specify the collatoral's contract address as the recipient address, so it would be helpful to check to make sure that is not the case

require(_recipient != address(this) && _recipient != address(0x0), "wrong recipient address")

QA3: https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/Collateral.sol#L49 The function assumes that an approval from msg.sender has been made before the function call. This assumption needs to be made explicit as a documentation for the function; otherwise, the function will revert .

QA4: https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/TokenSender.sol#L50 A range check is necessary for __priceMultiplier, for example, __priceMultiplier <= MULTIPLIER_DENOMINATOR.

QA5: https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/Collateral.sol#L85 Zero address check is needed so that fee will not be lost to the zero address of manager.

QA6: lock all contracts at the most recent version of solidity, 0.8.17.

QA7: https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/PrePOMarket.sol#L65 It is assumed that an allowance has been approved by msg.sender to PrePOMarket before the function call. Such assumption needs to be made explicit in documentation - otherwise, the function will revert.

QA8: https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/PrePOMarket.sol#L70-L71 It is assumed that both the collatoral token and the LongShortTokens have 18 decimals. This assumption needs to be explicit.

QA9: https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/PrePOMarket.sol#L65 Needs to check zero amount for _amount.

QA10. https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/PrePOMarket.sol#L42 Needs to check the contraint: _floorValuation < _ceilingValuation.

GA11 https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/PrePOMarket.sol#L65-L76 Lacks the check of expiry time: require( block.timestamp < _expiryTime, "market expires");

GA12:https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/TokenSender.sol#L42 Instead of using transfer(), which is not safe, use the safeTransfer instead: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol

GA13: https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/packages/prepo-shared-contracts/contracts/TokenSenderCaller.sol#L16 Zero address check is needed for the _treasury, otherwise, fee might be sent to the zero address and get lost forever.

G1. https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/packages/prepo-shared-contracts/contracts/NFTScoreRequirement.sol#L13 Dropping the first condition can save gas since the second condition already implies the first. Although this is a read function, it might be called by a write function.

 return getAccountScore(account) >= _requiredScore;

G2. https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/packages/prepo-shared-contracts/contracts/NFTScoreRequirement.sol#L22 Changing the arguments to calldata will save gas.

function setCollectionScores(IERC721[] calldata collections, uint256[] calldata scores) public virtual override

G3. https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/packages/prepo-shared-contracts/contracts/NFTScoreRequirement.sol#L35 Changing the arguments to calldata will save gas.

function removeCollections(IERC721[] calldata collections) public virtual override 

G4. https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/WithdrawHook.sol#L64 Change it to the following line to save gas:

globalAmountWithdrawnThisPeriod = globalAmountWithdrawnThisPeriod+_amountBeforeFee

G5. https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/PrePOMarket.sol#L95 After line 94, _collateralAllowanceBefore is actually equal to _expectedFee, so instead of introducing a new variable _collateralAllowanceBefore and calculating its values via collateral.allowance(), we can save gas by simply using _expectedFee.

G6: https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/PrePOMarketFactory.sol#L22 Changing memory variables to calldata vairables can save gas.

function createMarket(string calldata _tokenNameSuffix, string calldata _tokenSymbolSuffix, bytes32 longTokenSalt, bytes32 shortTokenSalt, address _governance, address _collateral, uint256 _floorLongPrice, uint256 _ceilingLongPrice, uint256 _floorValuation, uint256 _ceilingValuation, uint256 _expiryTime) external override onlyOwner nonReentrant {
  

G7 https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/PrePOMarketFactory.sol#L41 Changing memory variables to calldata vairables can save gas.

  function _createPairTokens(string calldata _tokenNameSuffix, string calldata _tokenSymbolSuffix, bytes32 _longTokenSalt, bytes32 _shortTokenSalt) internal returns (LongShortToken _newLongToken, LongShortToken _newShortToken) {
   

G8 https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/Collateral.sol#L53 Checking whether _fee is equal to zero will save gas:

if(_Fee > 0)  withdrawHook.hook(msg.sender, _baseTokenAmount, _baseTokenAmountAfterFee);

In this way, we do not have to check whether _fee > 0 again inside the hookd() function.

G9: https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/Collateral.sol#L80 might as well check whether _amount == 0 to save gas.