Rubicon contest - blockdev's results

Lines of code

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

Vulnerability details

Impact

Some ERC20 tokens return false on a failed transfer instead of reverting. For these tokens, it's required to check the return value to ensure a successful transfer. Failing to do that will lead to inaccurate accounting of funds.

Proof of Concept

Consider an ERC20 token which returns false on a failed transfer instead of reverting. Now, consider this line https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L602

underlyingToken.transfer(feeTo, _fee);

It will succeed even if the transfer failed since the return value was not checked to be true.

Tools Used

Recommended Mitigation Steps

Using OpenZeppelin's safeTransfer function will mitigate this issue, or wrap the statement in a require.

require(underlyingToken.transfer(feeTo, _fee));

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L298, https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L305, https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L309

Vulnerability details

Impact

A few ERC20 tokens like USDT have zero fee on transfer, but they can turn on the fee in future. In cases like these, when tokens are transferred, a fee is taken and then a lower amount is transferred. Thus BathToken will have a lower balance than intended.

Proof of Concept

Here's USDT transfer function taken from https://etherscan.io/address/0xdac17f958d2ee523a2206206994597c13d831ec7#code

function transfer(address _to, uint _value) public onlyPayloadSize(2 * 32) {
        uint fee = (_value.mul(basisPointsRate)).div(10000);
        if (fee > maximumFee) {
            fee = maximumFee;
        }
        uint sendAmount = _value.sub(fee);
        balances[msg.sender] = balances[msg.sender].sub(_value);
        balances[_to] = balances[_to].add(sendAmount);
        if (fee > 0) {
            balances[owner] = balances[owner].add(fee);
            Transfer(msg.sender, owner, fee);
        }
        Transfer(msg.sender, _to, sendAmount);
    }

Here, a lower amount is transferred to the receiver.

Now, as an example consider BathToken contract where transfer like these happen: https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L602

underlyingToken.transfer(feeTo, _fee);

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

underlyingToken.transferFrom(msg.sender, address(this), assets);

In both these cases, if token transfer fee is > 0, a lower amount will be received by the contract but the contract assumes that the entire amount is transferred leading to lower amount than intended in the contract.

Tools Used

Manual review

Recommended Mitigation Steps

Add a check on every transfer that ensures the intended amount is transferred by checking the before and after balance.

Summary

The documentation and whitepaper help immensely in understanding the motivation and code structure. There are helpful comments that also guide the code reader. That said, there are steps that can be taken to improve it further like complete Natspec documentation.

Issues

• Summary: Remove dead code Description: Dead code increases the burden on the reader and should be removed. IWETH interface is declared and never used, hence could be removed. Links: RubiconMarket.sol#262, IWETH.sol Recommendation: Remove both these interfaces.

• Summary: Upgrade Solidity compiler to at least 0.8.10. Description: Old Solidity versions have more compiler bugs, gas inefficient and require additional support like SafeMath. 0.8.10 was released a few months ago, and should be considered safe for production use. Additionally, 0.8.10 had a major gas optimization regarding external calls. If an external call returns a value, the compiler skips extcodesize check saving gas. Other benefits include skipping the SafeMath library. Recommendation: Upgrade Solidity to >=0.8.10.

• Summary: Use battle-tested libraries and contracts for common operations. Description: Codebase uses a custom written reentrancy guard and a guard to check that the caller is owner. This can be error-prone and often gas inefficient. OpenZeppelin provides all these functionalities which have been battle-tested and used in production by protocols handling millions and billions worth of user funds. to be secure. Links: synchronized (Reentrancy guard), auth Recommendation: Replace these modifiers with OZ’s ReentrancyGuard and onlyOwner modifier.

• Summary: Use Natspec documentation Description: Many public functions are missing Natspec documentation. Natspec commented code improves readability, is checked by the compiler and also integrated with etherscan. It helps the reader to understand the arguments and the return value of a function. RubiconMarket is a contract which specifically misses Natspec comments. Links: RubiconMarket.sol Recommendation: Add Natspec documentation for all public functions and variables.

• Summary: Use standard casing for code. Description: Structs should start with an uppercase character. Variables should start with a lowercase character. Public methods and variables should not start with an underscore. They are usually reserved for internal and private fields. The code has a mix of snake_case and camelCase style. Consider using camelCase everywhere. This just helps the reader to form correct assumptions based on the common understanding. Links: RubiconMarket.sol#L533-L547 Recommendation: Update the variables to conform to the common style of Solidity code, see description for details.

• Summary: An event can have at most 3 indexed parameters. Description: See Solidity documentation for events. It specifies an event can only have at most 3 indexed parameters. Links: RubiconMarket.sol#486 Recommendation: Remove “indexed” keyword from one event argument.

• Summary: Different types for the same parameter. Description: Different RubiconMarket function and events use different types for id (bytes32 vs uint256). Better to use uint256 across all events for ease of reading. Links: RubiconMarket.sol#256, RubiconMarket.sol#135 Recommendation: Use uint256 instead of bytes32 type.

• Summary: Can remove can_offer modifier as it always succeeds. Description: Removing redundant checks can save gas. In this case, can_offer modifier always succeeds which then doesn’t affect the logic. It also saves gas. Links: RubiconMarket.sol#221 Recommendation: Remove the can_offer modifier. However, this recommendation should be considered in context of future changes. For example, if there is a plan to add some logic in the modifier, then care should be taken to add it back. If there is a risk that adding the modifier back to all the places is missed, it’s better to let the modifier stay because L2 gas isn’t that big of an issue as on Ethereum mainnet.

• Summary: Users can make an offer with an amount less than dust. Description: There are several functions using which a user can offer a trade. A check is performed on the offer that the pay amount is >= the dust amount. But if [offer()](https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L612) is called matchingEnabled is false, this check is skipped, as SimpleMarket’s [offer()](https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L392) function does not perform this check. Links: RubiconMarket.sol# 612, RubiconMarket.sol#392 Recommendation: Add a dust related check to SimpleMarket’s offer() function.

• Summary: Use same name for semantically same function arguments. Description: Different buy related functions call the number of tokens to buy with different names (amount vs quantity). Links: RubiconMarket.sol#655, RubiconMarket.sol#272 Recommendation: Use the same name for both arguments.

• Summary: Future proof functions for reentrancy against any future commit which introduce external calls. Description: It’s a good practice to follow Checks-effects-interaction pattern even in the case where no external call is made. It safeguards against any changes made in the future which makes external calls. In this case, initialized is set to true at the end of the function. Links: BathHouse.sol#104 Recommendation: Set initialized = true immediately after the first require check performed in the function.

• Summary: Initialize the implementation contracts. Description: In the proxy pattern, an uninitialized implementation contract can be initialized by someone else taking over the contract. Even if it doesn’t affect the proxy contracts, it’s a good practice to initialize them yourself to prevent any mishap against unseen vulnerabilities. Recommendation: For the deployed contracts, execute the initialize() functions on the implementation contracts. There’s a risk that they might be frontrun, but it’s less likely since they are still uninitialized, and the frontrunner is not directly benefiting from executing the transaction itself. For future, consider calling OZ’s [_disableInitializers()](https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#initializing_the_implementation_contract) in the implementation contract’s constructor. Use the same name for both arguments.

• https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L184 Useless argument. Calldata size is also reduced which matters the most in gas reduction.
• No need to assign feeTo here, since https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L601 doesn’t transfer any token if it’s set to zero address which is the default value.
• This step can be removed, https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L33-L34. This contract never calls its own functions. Recommended to use OZ’s onlyOwner modifier.
• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L191 It can be made uint256. And instead of toggling between 0 and 1, it should be toggled between 1 and 2, since SSTORE is costlier when the state is 0. Alternatively, use OZ’s ReentrancyGuard.
• Use calldata args instead of memory. If the arguments are not being modified, it’s better to use calldata instead of memory since it’s cheaper. ​​​​https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L463-L469 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L442 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L578