search

code-423n4 / 2022-05-rubicon-findings

5 stars 2 forks source link

Multi-hop routes will leave a dust trail #432

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/RubiconDeFi/rubicon-protocol-v1/blob/6a0e1d89ff710f81428228d132e7d0c42de3a3cd/contracts/RubiconRouter.sol#L233

Vulnerability details

Impact

By calling RubiconMarket.buy(id, quantity) (as a consequence of L239 and L241) with 

quantity = currentAmount - currentAmount * expectedMarketFeeBPS / 10000

...the fee calculated by the buy function amounts to:

(currentAmount - currentAmount * expectedMarketFeeBPS / 10000)
 * expectedMarketFeeBPS / 10000

=
currentAmount * expectedMarketFeeBPS / 10000
- currentAmount * (expectedMarketFeeBPS / 10000)^2

Adding that fee to the quantity (that the buyer sells, possibly as part of a multihop route) results in:

currentAmount
- currentAmount * expectedMarketFeeBPS / 10000
+ currentAmount * expectedMarketFeeBPS / 10000
- currentAmount * (expectedMarketFeeBPS / 10000)^2

= currentAmount - currentAmount * (expectedMarketFeeBPS / 10000)^2

...which is the amount that will be spent by the contract from the buyer's wallet, spending over 99.99% of the (fillAmount) output from each loop iteration as the input of the next, and leaving a trail of dust in the buyer's wallet for each intermediary token.

The first token of a route is not affected by this issue because of LL229-230, meaning also that a swap requires the taker to have pay_amt + fee pay_gems available in his wallet and allowed to the router.

Recommended Mitigation Steps

Do not subtract the fee at this stage.

bghughes commented 2 years ago

Duplicate of #104