Rubicon contest - csanuragjain's results

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L475

Vulnerability details

Impact

Using a malicious targetPool, attacker can drain the contract of its funds

Proof of Concept

1. Attacker calls withdrawForETH with malicious targetPool

2. The malicious targetPool returns weth address on calling underlyingToken() which passes the require condition

IERC20 target = IBathToken(targetPool).underlyingToken();
        require(target == ERC20(wethAddress), "target pool not weth pool");

3. The malicious targetPool do nothing once transferFrom is called

IBathToken(targetPool).transferFrom(msg.sender, address(this), shares);

4. The malicious targetPool do nothing on call to withdraw and returns very high number of withdrawnWETH

withdrawnWETH = IBathToken(targetPool).withdraw(shares);

5. Contract simply transfer this withdrawnWETH to user. (Notice contract can have weth funds since depositWithETH does not return excess funds if passed accidentally by user)

WETH9(wethAddress).withdraw(withdrawnWETH);

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

Recommended Mitigation Steps

Apply whitelisting logic on targetPool

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L462

Vulnerability details

Impact

User can loss funds. If user accidentally sends extra ETH via msg.value while using depositWithETH or offerWithETH function then the extra amount of ETH is not refunded back to user

Proof of Concept

1. User calls depositWithETH function with amount as 5 and targetPool as X

2. User accidentally passed msg.value as 50 instead of 5

3. Deposit of amount 5 is success but user is not refunded back the excess amount which is 50-5=45

Note

Need to be fixed for offerWithETH function as well

Recommended Mitigation Steps

Send the extra ETH back to the user

if(msg.value>amount){
msg.sender.transfer(msg.value-amount);
}

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

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L519

Vulnerability details

Impact

User will loose funds if user accidentally pass route with only 1 value which is route[0]=X WETH while calling swapForETH or swapWithETH/swapEntireBalance/swap function

Proof of Concept

1. User calls swapForETH function with below params:

pay_amt=500
buy_amt_min=0
route[0]=WETH
expectedMarketFeeBPS=1

2. User will transfer 500+fees amount to the contract

require(
            ERC20(route[0]).transferFrom(
                msg.sender,
                address(this),
                pay_amt.add(pay_amt.mul(expectedMarketFeeBPS).div(10000))
            ),
            "initial ERC20 transfer failed"
        );

3. Now _swap function is called. This function will do nothing and loop will not run due to condition failure

for (uint256 i = 0; i < route.length - 1; i++)

// here since route.length - 1 is 1-1=0 so loop will not run as i=0 and 0<0 is false

4. Since currentAmount will be 0 and buy_amt_min is also 0 so require(currentAmount >= buy_amt_min, "didnt clear buy_amt_min"); will pass and 0 will be returned back

5. Swap is complete and user will not receive anything

Note:

The same need to be fixed for swapWithETH,swapEntireBalance,swap function as well

Recommended Mitigation Steps

Add below check

require(route.length>1, "Invalid route param");

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

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/peripheral_contracts/BathBuddy.sol#L38

constructor: Add below conditions to make sure params are correct

require(start>=block.timestamp, "Back date");
require(duration>0, "Invalid duration")

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L217

Function adminWriteBathToken: Add a new check to see newBathToken is allocated properly

require(newBathToken!=address(0), "Invalid address");

Note: same goes for setNewBathTokenImplementation function

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L253

Function setBathHouseAdmin: Changing admin should be 2 step process and also check for address(0)

Note: do this for setBathTokenBathHouse function as well

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L136

Function openBathTokenSpawnAndSignal: User can spawn new bath token with 0 liquidity. Add a check to see

require(initialLiquidityNew>0 && initialLiquidityExistingBathToken>0, "incorrect value");

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathPair.sol#L115

Function initialize: If attacker calls this function before bathHouse then attacker will get updated as bathHouse

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L260

Function setFeeBPS: _feeBPS should be in between 0 and 100. If it exceeds that then all withdraw will fail

require(_feeBPS>=0 && _feeBPS<=100, "Incorrect fees");

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathPair.sol#L310

Function getIndexFromElement: No need to keeping assigned variable. If index is found function will return within loop. If control is completing loop and coming outside to BathPair.sol#L318 then it already means that index was not found

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L144

Function openBathTokenSpawnAndSignal: No need to check getBathTokenfromAsset(newBathTokenUnderlying) == address(0) as this is already checked in _createBathToken

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L505

Function withdraw: No need to have owner argument. _withdraw is burning from msg.sender which eliminates the risk

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L221

Function can_offer: does not do anything and can be removed

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L471

Function isClosed: If this is meant to be hardcoded as false then better use a variable

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L455

Function depositWithETH: Check amount!=0

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L475

Function withdrawForETH: Check shares!=0

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L494

Function swapWithETH: Check pay_amt!=0

Note: do same for swapForETH and all other functions using pay_amt