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.
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
feeBPSto 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.sellAllAmountwith 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
tois set to zero.Consider checking
toargument against zero value, and in case replacing it withmsg.sender.RubiconRouter.sol#L547
commented-out code should be removed.