code-423n4 / 2021-09-sushitrident-findings

0 stars 0 forks source link

The functions `refundETH` and `unwrapWETH` is generalized-front-runnable #124

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

hrkrshnn

Vulnerability details

The functions refundETH and unwrapWETH is generalized-front-runnable

Context:

  1. https://github.com/sushiswap/trident/blob/9130b10efaf9c653d74dc7a65bde788ec4b354b5/contracts/TridentRouter.sol#L309
  2. https://github.com/sushiswap/trident/blob/9130b10efaf9c653d74dc7a65bde788ec4b354b5/contracts/TridentRouter.sol#L304

In particular, refundETH is very easily front-runnable, simply copy the calldata from mempool and get your transaction first. For unwrapWETH, the front-runner simply has to change the recipient address to their own address.

Although, this doesn't affect any part of the main contract logic, this possibility effectively makes the refund process for an average user almost impossible. The only certain way to get a refund is to use a service flashbots or taichi.

Recommended Mitigation Steps

There are two ways to resolve this.

  1. Make it clear in the documentation that one should not to call these functions directly and instead use a private mempool service.
  2. Make the withdrawal a two-step process. That is, a user can first claim, followed by the actual call.
maxsam4 commented 2 years ago

these functions should never be called directly. These are meant to be called in a batch. The contract should not have any remaining funds after the tx is over.

alcueca commented 2 years ago

Lack of documentation, sustained.

@maxsam4, would you be so kind as to mark all the issues that were raised due to the wardens not being aware that functions are intended to be called in a batch only, or from authorized frontends only (same thing, really)?