Rubicon contest - SmartSek's results

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L569-L572

Vulnerability details

Impact

The first depositor is able to steal subsequent deposits by directly transferring funds to the strategy. This attack vector is the same as the following audit report:

PrePO Audit Report

In general terms, the attacker deposits a small amount and will receive N shares. They then directly transfer a large amount of tokens directly to the strategy. Based on the share issuance logic, future deposits that are less than the transferred amount will receive 0 shares, effectively stealing their deposited tokens. The attacker is then able to withdraw all tokens in the strategy.

Proof of Concept

Exploit Walkthrough:

• Attacker deposits 1 wei of WETH, receives 1 wei of strategy tokens
• Attacker directly transfers 100 ether of WETH
• New user attempts to deposit 10 ether of WETH
• Based on the share issuance logic, the new user will receive 0 shares.

(totalSupply == 0) ? shares = assets : shares = (assets.mul(totalSupply)).div(_pool);

At this point, totalSupply = 1 wei. totalBefore will be roughly 100 WETH. valueAdded = 10 WETH.

Therefore:

1 * 10 / 100 = 0 strategy tokens

• The user's deposit is successful, meaning they have lost the deposited funds in exchange for 0 shares.
• The attacker can now withdraw all deposits using the only issued share.

Tools Used

Manual review

Recommended Mitigation Steps

Per the PrePO submission linked above, warden GreyArt does a great job explaining recommended mitigations.

• Uniswap V2 solved this problem by sending the first 1000 LP tokens to the zero address. The same can be done in this case i.e. when totalSupply() == 0, send the first min liquidity LP tokens to the zero address to enable share dilution.
• Ensure the number of shares to be minted is non-zero: require(_shares != 0, "zero shares minted");
• Create a periphery contract that contains a wrapper function that atomically calls initialize() and deposit()
• Call deposit() once in initialize() to achieve the same effect as the suggestion above.

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L451

Vulnerability details

##Impact

Funds could be unretrievable if msg.sender is a smart contract and its fallback function uses more than 2300 gas.

##Proof of Concept

The native transfer function has a gas limit of 2300 units. Solidity Docs

##Tools Used Manual Review

##Recommended Mitigation Steps

Consider using the native call function alongside a boolean check for its return value.

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L605

Vulnerability details

Impact

Unsafe transfer could lead to loss of funds for users.

Proof of Concept

Tokens that don’t implement the current EIP20 specifications might silently return false on a failed call instead of reverting. This would lead to user _shares being burned and him losing the power to claim his assets.

If the asset in question was USDT the transfer would always revert as USDT return void on transfers.

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend using safeTransfer and safeTransferFrom functions from OpenZeppelin’s SafeERC20 library that contain a return value check and can handle non-standard-compliant tokens.

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L245

Vulnerability details

Impact

Compromised admin can drain underlyingToken from BathToken.sol.

Proof of Concept

• Compromised admin calls setMarket and sets RubiconMarketAddress to malicious address.
• Compromised admin calls approveMarket.
• Now attacker in charge of the new malicious RubiconMarketAddress can reallocate funds from BathToken.sol to wherever he wants.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider implementing timelock constraints on functions with onlyAdmin modifiers.

Examples of similar issues ranked as high can be found here (issues H-07, H-09, H-10).

QA Report

[L-01] Outdated compiler version

pragma solidity =0.7.6;

Older compilers might be susceptible to bugs. A list of known compiler bugs and their severity can be found here: https://etherscan.io/solcbuginfo

I recommend changing the solidity version pragma to the latest version to enforce the use of an up-to-date compiler.

[L-02] Missing zero address check

If _proxyAdmin is accidentally set to zero the contract would have to be redeployed. BathHouse.sol#L122

---

Fees could accidentally be sent to zero address via feeTo BathHouse.sol#L330

---

If market, _feeTo or token are accidentally set to zero the contract would have to be redeployed. BathToken.sol#L181

We recommend implementing a zero address check

[L-03] approve is used instead of safeApprove or other preferred methods

Although safeApprove has been deprecated it is still preferable to approve.

Ideally safeIncreaseAllowance and safeDecreaseAllowance are recommended over safeApprove.

REFERENCE

BathHouse.sol#L165

[L-04] Use safeTransferFrom instead of transferFrom

Calling ERC20.transferFrom without handling the returned value is unsafe.

Consider using OpenZeppelin's SafeERC20 library with safe versions of transfer functions.

Example: BathToken.sol#L565

[L-05] initialize function can be frontrun

Contract might have to be redeployed if attacker calls initialize before initial deployer. To avoid having to redeploy the contract or potentially being hacked by the attacker I recommend using a factory to promptly call initialize after deployment and verify that the transaction succeeded.

Link below doesnt represent all instances: BathToken.sol#L181

NON-CRITICAL

[N-01] Inconsistent returns usage

Sometimes a named return is used instead of an unnamed one.

I recommend sticking with one or the other for better readability and consistency.

[N-02] Typo mark-to-marketed

Please fix to market-to-market BathToken.sol#L49

[N-03] Capitalised DOMAIN_SEPARATOR is not set as constant

Set DOMAIN_SEPARATOR as constant or remove capitalisation. https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L66

[N-04] Commented block of code

Block of code below should be deleted.

// /// @notice ERC-20 interface as derived from EIP-20
// contract ERC20 {
//     function totalSupply() public view returns (uint256);

//     function balanceOf(address guy) public view returns (uint256);

//     function allowance(address src, address guy) public view returns (uint256);

//     function approve(address guy, uint256 wad) public returns (bool);

//     function transfer(address dst, uint256 wad) public returns (bool);

//     function transferFrom(
//         address src,
//         address dst,
//         uint256 wad
//     ) public returns (bool);
// }

[N-05] Confusing semantics

After discussion with sponsor it was established that when calling placeOffer BathPair.sol is simultaneously and exclusively placing a bid AND ask. Not possible to place just a bid without an ask or vice versa. Therefore it would be best to remove or from The function that places a bid and/or ask code comment. BathToken.sol#L306

Gas Report

[G-01] Revert strings too long

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met.

BathHouse.sol#L162-178 BathToken.sol#L512)

[G-02] Redundant boolean declaration

require(IBathPair(_bathPairAddress).initialized() !=
  true, "BathPair already initialized");

Could be changed to:

require(IBathPair(
  !_bathPairAddress
).initialized(), "BathPair already initialized");

This would improve gas and readability.

BathHouse.sol#L242

[G-03] Modifier never used.

Please remove modifier onlyBathHouse in BathPair.sol as it is never used. BathPair.sol#L142-L145

[G-04] for loop gas optimization

for (uint256 index = 0; index < ids.length; index++) {
    uint256 _id = ids[index];
    scrubStrategistTrade(_id);
}

Gas could be saved by:

• Not initializing variable to default value of zero
• Caching array length
• Using a prefix (++i) instead of a postfix (i++)
• Unchecking increment count

Example:

size = ids.length;
for (uint256 index; index < size;) {
    uint256 _id = ids[index];
    scrubStrategistTrade(_id);
		unchecked { ++index; }
}