Rubicon contest - blackscale's results

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.

Lines of code

https://github.com/RubiconDeFi/rubicon-protocol-v1/blob/master/contracts/RubiconRouter.sol#L282

Vulnerability details

Impact

    function _swap(
        uint256 pay_amt,
        uint256 buy_amt_min,

maxAmount is a pay_amt denominated in token route[0] (=pay_gem)

buy_amt_min is a buy_amt denominated in token route[last] (=buy_gem)

buy_amt_min * expectedMarketFeeBPS / 1000 
is the fee denominated in token `route[last]`(=buy_gem)
-- takers pay fees with the token they receive from the swap

Line 282 subtracts a buy_gem amount (the fee) from a pay_gem amount, which makes no sense. Moreover, the fee is taken in pay_gems (from the tokens sent by the taker) so it has no place being mentioned in a buy_amt_min argument.

Recommended Mitigation Steps

Set buy_amt_min argument of _swap to buy_amt_min that was given as argument to this function (swapEntireBalance).

Judge has assessed an item in Issue #416 as Medium risk. The relevant finding follows:

Judge has assessed an item in Issue #416 as Medium risk. The relevant finding follows:

QA by blackscale

This report describes low severity issues by file, in their order of appearance in the code. It includes a mix of notes related to best practices and code clarity, and low impact or low probability issues.

RubiconMarket.sol#L282

The error message is misleading.

RubiconMarket.sol#L283

The error message is misleading.

RubiconMarket.sol#L298

If feeTo is set to 0x0, most ERC20 implementations (e.g. OpenZeppelin's) will revert on this transfer, making offers unbuyable.

RubiconMarket.sol#L479

Market can never be reopened once it has been stopped.

RubiconMarket.sol#L697

Although this function can not be called during a reentrancy, it is itself reentrant, via _sort. Consider adding a warning for future dev, so the risk does not compound during composition or evolution of the contract.

RubiconMarket.sol#L1231

This function allows an attacker with a compromised key to set an arbitrarily high fee. The fee will steal an arbitrary amount from buyers (with an infinite allowance) or up to the maximum allowance set for this contract. This is in some cases mitigated by the use of the router, which allows to set a minimum amount to control slippage, but any contract or user that interacts directly with RUbiconMarket remains vulnerable.

Consider bounding feeBPS to a sensible maximum value, or introducing a timelock to allow enough time for detection and reaction in case of wrongdoing.

RubiconMarket.sol#L1256

This function allows an attacker with a compromised key to immediately divert all future fees to an arbitrary address. When combined with the issue described on RubiconMarket.sol#L1231, this allows to siphon a large amount of user funds until the changes get noticed and a mitigation solution is found and implemented.

Consider introducing a timelock mechanism to allow enough time for detection and reaction, in order to better protect users.

RubiconMarket.sol#L1257

There is no check on the input not being zero. Considering the issue on RubiconMarket.sol#L298, such a mistake could have even worst consequences than simply not collecting fees until corrected.

Consider checking the input against the zero address.

RubiconRouter.sol#L243

The comment is misleading and makes wrong assumptions.

Calling RubiconMarket.sellAllAmount with a zero minimum amount means that all intermediary swaps of the route bypass the minimum. The rationale is that if the route ends up fulfilling the buy_amt_min requirement of the whole swap, we don't care what happened in between. It would also be very complex to calculate a sensible value for each step.

RubiconRouter.sol#L251

Will revert if to is set to zero.

Consider checking to argument against zero value, and in case replacing it with msg.sender.

RubiconRouter.sol#L547

commented-out code should be removed.

Gas optimization report

This report describes opportunity for gas optimization in the order of appearance in the code. It is not an exhaustive list of extreme gas optimization, but a series of quick wins that do not hinder code clarity (in some cases even enhance it), while having a perceptible net gain.

RubiconMarket.sol#L290

Spend depends on quantity, and this condition is superfluous. Proof below:

as per L280, spend = quantity * buy_amt / pay_amt so spend > buy_amt (L290) is equivalent to: (quantity * buy_amt / pay_amt) > buy_amt quantity / pay_amt > 1 quantity > pay_amt = L289

Being equivalent, there is no case where the condition on L289 evaluates to false and L290 would evaluate to true, so evaluating both is a waste of gas.

Therefore, this condition is redundant and can be removed, sparing a boolean operation, 2 memory reads and a comparison check on most calls to the buy function.

RubiconMarket.sol#L299

Error message is long and wasteful in terms of gas both at deployment time (contributing to reaching the 24k limit faster) and at runtime.

Recommendation: A good strategy to minimize gas costs while maintaining functionality is to encode error messages in the following way.

1. copy error text
2. replace text with 4char code, where the first 2 chars define the source of the error, e.g. by using the contract initials RM for RubiconMarket, and the last 2 chars are the incremental id of the message. RM00 to RM99 allows for 100 different errors in the contract.
3. (optional) if you have tests that check on the error message, replace all occurrences of errorText in test folder with errorCode.
4. paste error text in a comment below the require statement.
5. (optional) copy and paste "// errorCode: errorText" at top of contract

The rationale is that these error messages are meant for developers or code to consume, not for end-users, which might require more verbose output anyway, possibly even localized. Such encoding is still human-readable and very useful for developers, while minimizing gas cost and maintaining full functionality in terms of testing and frontend integration.

RubiconMarket.sol#L306

Long error message, see note at RubiconMarket.sol#L299

RubiconMarket.sol#L310

Long error message, see note at RubiconMarket.sol#L299

RubiconMarket.sol#L571

Long error message, see note at RubiconMarket.sol#L299

RubiconMarket.sol#L574

Long error message, see note at RubiconMarket.sol#L299

RubiconMarket.sol#L618

Both paths of this function end up calling super.offer(...) eventually, which is already protected by the synchronized modifier. This requirement is redundant.

Recommendation: Remove the superfluous requirement to save on gas.

RubiconMarket.sol#L645

This function calls super.offer(...), which is already protected by the synchronized modifier. This requirement is redundant.

Recommendation: Remove the superfluous requirement to save on gas.

RubiconMarket.sol#L661

All paths of this function end up calling super.buy(...) eventually, which is already protected by the synchronized modifier. This requirement is redundant.

Recommendation: Remove the superfluous requirement to save on gas.

RubiconMarket.sol#L684

This function calls super.cancel(...), which is already protected by the synchronized modifier. This requirement is redundant.

Recommendation: Remove the superfluous requirement to save on gas.

RubiconMarket.sol#L701

Message is a duplicate and will be ambiguous regarding its emission source. Consider encoding as described in note RubiconMarket.sol#L299

RubiconMarket.sol#L714

Message is a duplicate and will be ambiguous regarding its emission source. Consider encoding as described in note RubiconMarket.sol#L299

RubiconMarket.sol#L835

Message is a duplicate and will be ambiguous regarding its emission source. Consider encoding as described in note RubiconMarket.sol#L299

RubiconMarket.sol#L874

Message is a duplicate and will be ambiguous regarding its emission source. Consider encoding as described in note RubiconMarket.sol#L299