Rubicon contest - reassor's results

Lines of code

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

Vulnerability details

Impact

Contract BathToken.sol implements distributeBonusTokenRewards function that allows distributing non-underlying bath token incentives to pool withdrawers. In case of rewardsVestingWallet being set implementation triggers release function of rewardsVestingWallet. The issue is that rewardsVestingWallet is uninitalized (set to zero address) and cannot be set in any way which means that there is no way of executing rewardsVestingWallet.release logic. The result is that function distributeBonusTokenRewards does not work which leads to loss of bonus token rewards thus effectively loss of funds.

Proof of Concept

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to initialize value of rewardsVestingWallet in initialize or add additional function for setting its value.

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L1231-L1234 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L295-L300

Vulnerability details

SimpleMarket - owner can steal funds via fee

Impact

Contract RubiconMarket implements function buy that accepts given quantity of an offer and transfers funds from caller/taker to offer maker, and form market to caller/taker. The logic implements the fee that is by default set to 20 base points which is 0.2%. The issue is that the owner can change value of feeBPS at any time using setFeeBPS and can its value as high as 100%.

Exploit Scenario:

1. User(s) can see that the fee is 0.2% and he decides to buy.
2. Owner increases feeBPS to 100% via setFeeBPS (or runs front-running attack).
3. setFeeBPS transaction is being included before user's buy transaction.
4. Stolen funds via feeBPS are sent to feeTo address.

This issue is also relevant in case of owner not being a malicious actor. User accepts some kind of fee that is visible as a getFeeBPS() for example 0.2%. Then owner just change the feeRate to 3%. User didn't expect that change and effectively loses funds.

Proof of Concept

• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L1231-L1234
• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L295-L300

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add hard limit for the feeBPS - for example maximum can be set to 3%. In addition it is recommended to add timelock for changing feeBPS via setFeeBPS so it will be not possible to launch front-running attack.

1. Cache array length outside of loop

Impact

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop.

Proof of Concept

RubiconRouter.sol:169:        for (uint256 i = 0; i < route.length - 1; i++) {
RubiconRouter.sol:227:        for (uint256 i = 0; i < route.length - 1; i++) {
rubiconPools/BathToken.sol:635:            for (uint256 index = 0; index < bonusTokens.length; index++) {
rubiconPools/BathPair.sol:311:        for (uint256 index = 0; index < array.length; index++) {
rubiconPools/BathPair.sol:582:        for (uint256 index = 0; index < ids.length; index++) {

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to cache the array length outside of for loop.

2. Long revert error messages

Impact

Shortening revert error messages to fit in 32 bytes will decrease gas costs for deployment and gas costs when the revert condition has been met.

Proof of Concept

RubiconMarket.sol:306:            "_offer.buy_gem.transferFrom(msg.sender, _offer.owner, spend) failed - check that you can pay the fee"
RubiconMarket.sol:571:        require(isActive(id), "Offer was deleted or taken, or never existed.");
RubiconMarket.sol:574:            "Offer can not be cancelled because user is not owner, and market is open, and offer sells required amount of tokens."
RubiconRouter.sol:338:            "must send as much ETH as max_fill_withFee"
RubiconRouter.sol:392:            "didnt send enough native ETH for WETH offer"
RubiconRouter.sol:446:            "trying to cancel a non WETH order"
RubiconRouter.sol:506:            "must send enough native ETH to pay as weth and account for fee"

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to decrease revert messages to maximum 32 bytes. Alternatively custom error types should be used.

3. ++i/--i costs less gas compared to i++, i += 1, i-- or i -= 1

Impact

++i or --i costs less gas compared to i++, i += 1, i-- or i -= 1 for unsigned integer as pre-increment/pre-decrement is cheaper (about 5 gas per iteration).

Proof of Concept

RubiconMarket.sol:436:        last_offer_id++;
RubiconMarket.sol:1165:        _span[address(pay_gem)][address(buy_gem)]++;
RubiconRouter.sol:85:        for (uint256 index = 0; index < topNOrders; index++) {
RubiconRouter.sol:169:        for (uint256 i = 0; i < route.length - 1; i++) {
RubiconRouter.sol:227:        for (uint256 i = 0; i < route.length - 1; i++) {
rubiconPools/BathToken.sol:635:            for (uint256 index = 0; index < bonusTokens.length; index++) {
rubiconPools/BathToken.sol:733:                        nonces[owner]++,
rubiconPools/BathPair.sol:206:        last_stratTrade_id++;
rubiconPools/BathPair.sol:311:        for (uint256 index = 0; index < array.length; index++) {
rubiconPools/BathPair.sol:427:        for (uint256 index = 0; index < quantity; index++) {
rubiconPools/BathPair.sol:480:        for (uint256 index = 0; index < quantity; index++) {
rubiconPools/BathPair.sol:582:        for (uint256 index = 0; index < ids.length; index++) {
RubiconMarket.sol:1197:        _span[pay_gem][buy_gem]--;

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to use ++i or --i instead of i++, i += 1, i-- or i -= 1 to increment value of an unsigned integer variable.

4. No need to explicitly initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for addresses). Explicitly initializing it with its default value is an anti-pattern and waste of gas.

Proof of Concept

rubiconPools/BathPair.sol:310:        bool assigned = false;
RubiconMarket.sol:990:        uint256 old_top = 0;
RubiconRouter.sol:82:        uint256 lastBid = 0;
RubiconRouter.sol:83:        uint256 lastAsk = 0;
RubiconRouter.sol:85:        for (uint256 index = 0; index < topNOrders; index++) {
RubiconRouter.sol:168:        uint256 currentAmount = 0;
RubiconRouter.sol:169:        for (uint256 i = 0; i < route.length - 1; i++) {
RubiconRouter.sol:226:        uint256 currentAmount = 0;
RubiconRouter.sol:227:        for (uint256 i = 0; i < route.length - 1; i++) {
rubiconPools/BathToken.sol:635:            for (uint256 index = 0; index < bonusTokens.length; index++) {
rubiconPools/BathPair.sol:311:        for (uint256 index = 0; index < array.length; index++) {
rubiconPools/BathPair.sol:427:        for (uint256 index = 0; index < quantity; index++) {
rubiconPools/BathPair.sol:480:        for (uint256 index = 0; index < quantity; index++) {
rubiconPools/BathPair.sol:582:        for (uint256 index = 0; index < ids.length; index++) {

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to remove explicit initializations with default values.

5. Use != 0 instead of > 0 for unsigned integer comparison

Impact

When dealing with unsigned integer types, comparisons with != 0 are cheaper that with > 0.

Proof of Concept

RubiconMarket.sol:233:        return offers[id].timestamp > 0;
RubiconMarket.sol:400:        require(pay_amt > 0);
RubiconMarket.sol:402:        require(buy_amt > 0);
RubiconMarket.sol:837:        while (pay_amt > 0) {
RubiconMarket.sol:876:        while (buy_amt > 0) {
RubiconMarket.sol:918:            if (pay_amt > 0) {
RubiconMarket.sol:942:            if (buy_amt > 0) {
RubiconMarket.sol:985:        require(id > 0);
RubiconMarket.sol:1002:        require(id > 0);
RubiconMarket.sol:1063:        while (_best[address(t_buy_gem)][address(t_pay_gem)] > 0) {
RubiconMarket.sol:1099:            t_buy_amt > 0 &&
RubiconMarket.sol:1100:            t_pay_amt > 0 &&
RubiconMarket.sol:1175:        require(_span[pay_gem][buy_gem] > 0);
RubiconMarket.sol:1217:        while (uid > 0 && uid != id) {
RubiconRouter.sol:354:        if (delta > 0) {
rubiconPools/BathToken.sol:634:        if (bonusTokens.length > 0) {
rubiconPools/BathPair.sol:232:            if (askDelta > 0) {
rubiconPools/BathPair.sol:252:            if (bidDelta > 0) {
rubiconPools/BathPair.sol:333:            (askNumerator > 0 && askDenominator > 0) ||
rubiconPools/BathPair.sol:334:                (bidNumerator > 0 && bidDenominator > 0),
rubiconPools/BathPair.sol:515:        if (assetRebalAmt > 0) {
rubiconPools/BathPair.sol:523:        if (quoteRebalAmt > 0) {
rubiconPools/BathPair.sol:597:        if (fillCountA > 0) {
rubiconPools/BathPair.sol:611:        if (fillCountQ > 0) {
rubiconPools/BathHouse.sol:111:        require(_reserveRatio > 0);
rubiconPools/BathHouse.sol:281:        require(rr > 0);

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to use != 0 instead of > 0.

6. Pack integer values

Impact

Packing integer variables into storage slots saves gas.

Proof of Concept

BathPair.sol:

• pack timestamp as uint64 - https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L76

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to pack listed values in order to consume less storage and thus gas.