Rubicon contest - dipp's results

Lines of code

RubiconRouter.sol#L475-L492

Vulnerability details

Impact

In the withdrawForETH function in RubiconRouter.sol, the targetPool may be any contract that implements the IBathToken interface and returns wethAddress as its underlying token. The withdrawnWETH amount could be set to the RubiconRouter.sol contract's WETH balance so that the contract's entire WETH balance is withdrawn, as long as the tagetPool does not transfer any WETH to RubiconRouter.sol. The caller of the withdrawForETH function would then receive the withdraw amount.

Proof of Concept

    function withdrawForETH(uint256 shares, address targetPool)
        external
        payable
        returns (uint256 withdrawnWETH)
    {
        IERC20 target = IBathToken(targetPool).underlyingToken();
        require(target == ERC20(wethAddress), "target pool not weth pool");
        require(
            IBathToken(targetPool).balanceOf(msg.sender) >= shares,
            "don't own enough shares"
        );
        IBathToken(targetPool).transferFrom(msg.sender, address(this), shares);
        withdrawnWETH = IBathToken(targetPool).withdraw(shares);
        WETH9(wethAddress).withdraw(withdrawnWETH);

        //Send back withdrawn native eth to sender
        msg.sender.transfer(withdrawnWETH);
    }

1. Let shares be equal to the contracts WETH balance.

2. The malicious targetPool contract returns the wethAddress as the underlying token on line 480.

3. targetPool returns the max uint256 value for its balanceOf function to pass the require condition on line 483 for any value of shares.

4. The transferFrom on line 486 does not have to do anything and its withdraw function should return the WETH balance of RubiconRouter.sol.

5. The RubiconRouter.sol contract will then withdraw ETH equal to the withdrawWETH amount, which should be equal to the contract's WETH balance.

6. The caller of the withdrawForETH function receives the withdraw ETH without providing any WETH.

Recommended Mitigation Steps:

Check the contract's WETH balance before the caller is supposed to send the WETH and after the WETH is sent to confirm the contract has received enough WETH from the caller.

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

Lines of code

RubiconRouter.sol#L325-L358 RubiconRouter.sol#L383-L409 RubiconRouter.sol#L455-L472

Vulnerability details

Impact

When calling a number of functions in RubiconRouter.sol that require ETH, msg.value is allowed to be more than the actual amount needed. Only the amount needed is converted to WETH and the remaining ETH is not sent back to the user.

Proof of Concept

buyAllAmountWithETH function example:

1. A user calls buyAllAmountWithETH and sends 1000 ETH to the contract.

2. If the max_fill_amount_with_fee is 500, only 500 ETH is converted to WETH.

3. RubiconRouter.sol now has 500 WETH and 500 ETH.

4. Lets say only 400 WETH is used in the buyAllAmount trade in RubiconMarket.sol, then the remaining WETH is 100.

5. The user receives the remaining WETH refund on line 356.

6. The remaining 500 ETH stays in the contract and is not sent back to the user.

offerWithETH function example:

1. A user calls offerWithETH with a pay_amt of 500 and msg.value 1000 ETH.

2. Only the pay_amt amount is converted to WETH and the rest of the ETH remains in the contract.

depositWithETH function example:

1. A user calls depositWithETH with a pay_amt of 500 and msg.value 1000 ETH.

2. Only the pay_amt amount is converted to WETH on line 468.

3. The shares gained by the targetPool is sent to the caller on line 471 but the remaining 500 ETH is not.

Recommended Mitigation Steps

Consider changing the require conditions from msg.value >= to msg.value == so that users are not allowed to send too much ETH.

1. Potential underflows

Line References

BathPair.sol#L608

BathPair.sol#L609

BathPair.sol#L622

BathPair.sol#L623

Impact

The deductions on lines 608, 609, 622 and 623 are not protected against underflows. Use the SafeMath sub function instead, so that if an underflow occurs the transaction reverts.

2. Several instances of unchecked transfers

Line References

RubiconRouter.sol#L202

RubiconRouter.sol#L251

RubiconRouter.sol#L274

RubiconRouter.sol#L303

RubiconRouter.sol#L320

RubiconRouter.sol#L348

RubiconRouter.sol#L366

RubiconRouter.sol#L377

RubiconRouter.sol#L406

RubiconRouter.sol#L419

RubiconRouter.sol#L471

RubiconRouter.sol#L486

BathPair.sol#L601

BathPair.sol#L615

BathToken.sol#L353

BathToken.sol#L357

BathToken.sol#L565

BathToken.sol#L605

BathBuddy.sol#L114

Impact

Some tokens return boolean values instead of reverting on failure and should be checked everytime transfer or transferFrom are called. Put the transfer in a require so that a failed transaction that returns false reverts.