code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

depositAndTrade function seems to be incomplete - the tokenOutput from _swapRouter is currently owned by DepositTradeHelper account and needs to be transferred back to msg.sender who initiated this transaction. Since this contract doesn't seem to be part of core contracts of protocol (although listed in audit scope), I've marked it as MEDIUM risk.

Current implementation does not use return value of _swapRouter.exactInputSingle() in line 33. As per UniswapV3 docs, exactInputSingle function returns a token output value amountOutput as below

  function exactInputSingle(
    struct ISwapRouter.ExactInputSingleParams params
  ) external returns (uint256 amountOut)

Since such tokens rightfully belong to msg.sender, a transfer of the said token needs to be implemented to complete the loop (with a nonReentrant) modifier

Proof of Concept

Bob signs a offchain message to approve USDC in his wallet to be spent by DepositTradeHelper. Contract executes depositAndTrade function to swap out USDC for long/short tokens of say SpaceX prePO. Bob does not see the tokens in his wallet even after successful execution of the transaction

Tools Used

Recommended Mitigation Steps

Helper should complete the loop -> by transferring tokens returned by Uniswap V3 router to msg.sender. Consider code below as a suggested replacement for Line 33 in existing code base


    uint256 tokenOutput = _swapRouter.exactInputSingle(exactInputSingleParams); // amountOut output is not used

    if (tokenOutput > 0) {
      bool success = IERC20(tradeParams.tokenOut).transfer(msg.sender, tokenOutput);
      require(success, "swapped token transfer failed");
    }

Not to forget, this function needs to use the nonReentrant modifier

Picodes commented 1 year ago

The recipient of the trade is msg.sender, meaning the output of the trade is transferred directly to msg.sender

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Invalid

code-423n4 / 2022-12-prepo-findings