Upgraded Q -> H from 158 [1656140803140]

[code423n4]

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

[HickupHH3]

swapEntireBalance() would work unexpectedly. contracts\RubiconRouter.sol#L280-286 _swap( maxAmount, maxAmount.sub(buy_amt_min.mul(expectedMarketFeeBPS).div(10000)), route, expectedMarketFeeBPS, msg.sender );

Impact The second parameter of _swap function is buy_amt_min but it used the wrong parameter. In this case, _swap function will work wrongly.

Proof of Concept maxAmount is for paying asset(route[0]) and buy_amt_min is for buying asset(route[last]). So it's impossible to do sub operation with these values at L282.

Recommended Mitigation Steps We need to calculate the estimated fee like issue 5. You can replace L279-L286 like this. (pseudocode) You can move require() to the beginning of the function.

uint256 route_length = route.length; require(route_length >= 2, "invalid route length"); uint256 pay_amt = maxAmount; for(uint i; i < route_length - 1; ++i) { pay_amt.sub(pay_amt.mul(expectedMarketFeeBPS).div(10000 + expectedMarketFeeBPS)); } return _swap( pay_amt, buy_amt_min, route, expectedMarketFeeBPS, msg.sender );

dup of #52