Upgraded Q -> M from 158 [1656141434980]

[code423n4]

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

[HickupHH3]

7. maxBuyAllAmount() would work unexpectedly. contracts\RubiconRouter.sol#L290-304 function maxBuyAllAmount( ERC20 buy_gem, ERC20 pay_gem, uint256 max_fill_amount ) external returns (uint256 fill) { //swaps msg.sender's entire balance in the trade uint256 maxAmount = ERC20(buy_gem).balanceOf(msg.sender); fill = RubiconMarket(RubiconMarketAddress).buyAllAmount( buy_gem, maxAmount, pay_gem, max_fill_amount ); ERC20(buy_gem).transfer(msg.sender, fill); }

Impact It calls the buyAllAmount() function of RubiconMarket contract with wrong parameters so this function would be useless.

Proof of Concept First, it gets maxAmount of buy_gem from the sender but it's useless because the sender is going to buy buy_gem using pay_gem. So we need to get the available amount of pay_gem from the sender and estimate the buy amount of buy_gem using getBuyAmount() of RubiconMarket contract. (RubiconMarket.sol#L909)

Second, the function has a max_fill_amount parameter but it's useless because this function tries to use the entire balance of pay_gem. We can just calculate that value from the available balance of pay_gem by reducing the fee. In order to estimate fees, we need to add an expectedMarketFeeBPS parameter like other functions even though I think I can get feeBPS from RubiconMarket directly.

Recommended Mitigation Steps I will remove max_fill_amount parameter and add expectedMarketFeeBPS instead. You can implement the function like this. (pseudocode)

function maxBuyAllAmount( ERC20 buy_gem, ERC20 pay_gem, uint256 expectedMarketFeeBPS ) external returns (uint256 fill) { //swaps msg.sender's entire balance in the trade uint256 maxAmount = ERC20(pay_gem).balanceOf(msg.sender); uint256 pay_amt = maxAmount.sub(maxAmount.mul(expectedMarketFeeBPS).div(10000 + expectedMarketFeeBPS)); uint256 buy_amt = RubiconMarket(RubiconMarketAddress).getBuyAmount(buy_gem, pay_gem, pay_amt); fill = RubiconMarket(RubiconMarketAddress).buyAllAmount( buy_gem, buy_amt, pay_gem, maxAmount ); ERC20(buy_gem).transfer(msg.sender, fill); }

maxSellAllAmount() would work unexpectedly. contracts\RubiconRouter.sol#L307-321 function maxSellAllAmount( ERC20 pay_gem, ERC20 buy_gem, uint256 min_fill_amount ) external returns (uint256 fill) { //swaps msg.sender entire balance in the trade uint256 maxAmount = ERC20(buy_gem).balanceOf(msg.sender); fill = RubiconMarket(RubiconMarketAddress).sellAllAmount( pay_gem, maxAmount, buy_gem, min_fill_amount ); ERC20(buy_gem).transfer(msg.sender, fill); } Impact It calls the sellAllAmount() function of RubiconMarket contract with wrong parameters so this function would be useless.

Proof of Concept This issue is similar to the previous one. First, it gets maxAmount of buy_gem from sender but it's useless because the sender is going to buy buy_gem using pay_gem. So we need to get the available amount of pay_gem from the sender. And we need to know expectedMarketFeeBPS in order to estimate fees.

Recommended Mitigation Steps I will add the expectedMarketFeeBPS parameter. You can implement the function like this. (pseudocode)

function maxSellAllAmount( ERC20 pay_gem, ERC20 buy_gem, uint256 min_fill_amount, uint256 expectedMarketFeeBPS ) external returns (uint256 fill) { //swaps msg.sender entire balance in the trade uint256 maxAmount = ERC20(pay_gem).balanceOf(msg.sender); uint256 pay_amt = maxAmount.sub(maxAmount.mul(expectedMarketFeeBPS).div(10000 + expectedMarketFeeBPS)); fill = RubiconMarket(RubiconMarketAddress).sellAllAmount( pay_gem, pay_amt, buy_gem, min_fill_amount ); ERC20(buy_gem).transfer(msg.sender, fill); }

dup of #282