Rubicon contest - VAD37's results

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/rubiconPools/BathToken.sol#L756-L759

Vulnerability details

EIP-4626 have fatal flaw that when totalAssets can be manipulated then so token share price.

Impact

User can create new BathToken and inflate share price so high that deposit will always return 0 share regardless of amount token transfer. Basically, give free token to first user deposit in BathToken pool

Proof of Concept

• User create BathToken for DAI through BathHouse.openBathTokenSpawnAndSignal() with initialLiquidity = 1e1 DAI.
• EIP-4626 share price of BathToken now is 1:1 (1 DAI = 1 share BathToken). TotalSupply = 1e1 and TotalAssets = 1e1
• User transfer 1000e18 DAI to BathToken pool directly (no mint or deposit). Pool will take this as rewards with underlyingBalance() return current balance as 1000e18.
• The share price now is 1:1000e18 (1000 DAI for 1 share BathToken). TotalSupply = 1e1 and TotalAssets = 1000e18
• Now everyone deposit or mint to this pool will call convertToShares formular assetsTransfered * totalSupply / totalAssets. Which always return 0 if transfer less than 1000 DAI.
• First user deposit have 1 share now receive token from other deposit for free.
• If other user transfer 1600 DAI, the math still round down number and give second user 1 share. And first user share now worth 1300 DAI

Recommeneded migration steps

The current easiest workaround is new BathToken can only be created by owner. Owner can create pool with large amount of initial liquidity, enough to prevent share price manipulation.

The underlying problem still is totalAssets can be manipulated by anyone then so token share price.

• Replace BathToken.underlyingBalance() return balanceOf current address with a global variable _totalAsset
• Every time user deposit, withdraw, mint, redeem. Update _totalAsset amount.
• When receive rewards from BathPair.sol. Create new onlyPair function to update new rewards balance.
• Update the new balance of IERC20(underlyingToken).balanceOf(address(this)) to _totalAsset. Depositor will get their new share price fairly

This help prevents anyone from manipulate share price of token through totalAssets and only strategist can do so.

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/521d50b22b41b1f52ff9a67ea68ed8012c618da9/contracts/rubiconPools/BathToken.sol#L565

Vulnerability details

The majority of project use interface call IERC20(tokenAddress).transfer() to transfer token without checking return value. With the exception of RubiconMarket.sol

Impact

• Does not support non-standard ERC20 token like USDT.
• Failure to check if token is successful transferred or not on every function.

EIP-20 implement function transfer(address _to, uint256 _value) public returns (bool success) have a boolean return false value on failed transfer. Majority of ERC20 token never return false value. So most of the time people can get away with not checking return value.

Proof of Concept

• USDT use non-standard ERC20 transfer version that does not return any value on transfer. Contract call to coin like USDT always revert.
• Since BathToken.sol ignore transfer return value. If the underlying token return false somehow, attacker can manipulate that pool to deposit no token but still receive share from the pool. (Unlikely to happen)

Recommended migration step

Use safeTransfer library by OpenZeppelin.

Replace all ERC20().transfer and transferFrom with library call for all core files (RubiconMarket.sol, RubiconRouter.sol, BathToken.sol, etc...)