Upgraded Q -> H from 158 [1656140317935]

[code423n4]

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

[HickupHH3]

Wrong fee calculation 1 contracts\RubiconRouter.sol#L174-L176 contracts\RubiconRouter.sol#L232-234

currentAmount.sub( currentAmount.mul(expectedMarketFeeBPS).div(10000) )

Impact Current logic estimates more fee than expected so traders wouldn't spend all of the pay_amt they requested.

Proof of Concept We know that "fee" is calculated using the amount "spend" from contracts\RubiconMarket.sol#L296. uint256 fee = mul(spend, feeBPS) / 10000; So if X is total "spend" amount, fee will be X feeBPS / 10000, total amount of funds(let's call Y) Y = X + X feeBPS / 10000 = X (10000 + feeBPF) / 10000 So when we calculate X from Y, X = Y 10000 / (10000 + feeBPS) = Y - Y feeBPS / (10000 + feeBPS) But currently it caluates like this. X = Y - Y feeBPS / 10000

Recommended Mitigation Steps You need to divide by 10000 + expectedMarketFeeBPS, not only 10000 You can modify #L175, #L233 like this.

currentAmount.mul(expectedMarketFeeBPS).div(10000 + expectedMarketFeeBPS)

Wrong fee calculation 2 contracts\RubiconRouter.sol#L205 Impact Current logic adds fee only once so it would be failed because of insufficient funds when the length of route is greater than 2.

Proof of Concept This function can go through several routes so you need to add fees several times also. Otherwise, the function will be reverted because available funds are less than expected after taking fees.

Recommended Mitigation Steps You can replace L202-L206 like this. (pseudocode)

uint256 route_length = route.length; require(route_length >= 2, "invalid route length"); uint256 pay_amt_total = pay_amt; for(uint i; i < route_length - 1; ++i) { pay_amt_total.add(pay_amt_total.mul(expectedMarketFeeBPS).div(10000)); } ERC20(route[0]).transferFrom( msg.sender, address(this), pay_amt_total );

dup of #104