Biconomy Hyphen 2.0 contest - 0xngndev's results

Low-impact Report

Low Impact Issues

Issue #1: Gas is not being accounted properly

Impact

There’s two issues here but I bundled them up into one because they’re related:

• In LiquidityPool#sendFundsToUser, initialGas is calculated using gasLeft() and then after some calculations baseGas is added, which is correct as this represents the intrinsic cost of calling a function. However, you are not accounting for the 1/64 cost of calling the function (EIP-150), which only allows to pass 63/64 of the available gas to the different call frames. This means that initialGas, or gasLeft() will be: AvailableGas * 63/64 - 21000
• In LiquidityPool#sendFundsToUser, everything that happens after calculating amountToTransfer is not accounted into the gas spent and I don’t think there’s a way to actually account for it other than add a sort of extraGas state variable, which is wonky. This is not great because you are executing external calls afterwards, but as I said I don’t think there’s a clean way to fix it. However, there’s an interesting thing that I believe should be brought up if you want to measure gas usage as precisely as the evm allows us. In LiquidityPool#getAmountToTransfer, totalGasUsed doesn’t take into account the update of the following mappings:
  • gasFeeAccumulatedByToken[tokenAddress]
  • gasFeeAccumulated[tokenAddress][_msgSender]
  These are mappings storing information on storage, meaning that whoever updates these mappings first (setting them from 0 to some value), will have to pay two cold SSTORES (20000 gas) and these will go unaccounted, so what he’s owed will be off by 40000 + all the other small logic that isn’t accounted.

Just as a sidenote, although I don’t think it applies here but you may find it interesting: gas refunds are paid post-transaction, so there’s no precise way to account for them inside the logic of a contract. gasLeft() will not catch them.

Steps to Mitigate

1. The first issue can be addressed by doing: initialGas = (gasLeft() * 64)/63

2. The second issue, which is harder to handle, I can only that you can:

• Create an unaccountedGas state variable with a setter that works as a sort of bump/bonus to account for all the logic after LiquidityPool#getAmountToTransfer#totalGasUsed calculation. But this doesn’t take into account the first caller having to pay for the SSTORES. To tackle that, the only thing I can think of is to warm the slots before totalGasUsed is calculated. You would do something along the lines of:

gasFeeAccumulatedByToken[tokenAddress] += 1;
gasFeeAccumulated[tokenAddress][_msgSender] += 1;
totalGasUsed = initialGas - gasLeft();
//..code
gasFeeAccumulatedByToken[tokenAddress] += gasFee - 1;
gasFeeAccumulated[tokenAddress][_msgSender] += gasFee - 1;

which is hacky, but it would properly account the 40000 extra gas of the first caller at the cost of an extra 200 gas cost per function call (warm SSTORES cost 100 each).

Non-Critical Issues

Issue #2: Error messages’ style is not consistent accross contracts

Summary

In ExecutorManager.sol, errors messages have the following style:

• executor address can not be 0

While in the rest of the contracts, they have this style:

• ERR__TO_IS_ZERO

Issue #3: Typos

Two instances:

• It should be recipient in this line
• after is misspelled in the natspec: here

Issue #4: Commented out code

Two instances:

1. Here

2. Here

Issue #1: ExecutorManager’s onlyExecutor is not used

Impact

The modifier onlyExecutor in ExecutorManager.sol is not used in the repository. It can be removed. Only LiquidityPool.sol uses a similar modifier with the same name, but it has it’s own implementation.

Steps to Mitigate

Remove onlyExecutor from ExecutorManager.sol to save on deployment costs.

Issue #2: Ownable is partially used in ExecutorManager

Impact

Can reduce costs if you implement your partial version of Ownable with only the variables, modifiers and function you need.

Steps to Mitigate

Create a new contract which is a partial implementation of Ownable

Issue #3: Long error messages

Impact

This is a common gas optimization possibility that appears in a lot of contracts. For this reason, wardens have added it to a list of common gas optimizations. I’ll link to the explanation, which is as detailed as it gets:

https://github.com/byterocket/c4-common-issues/blob/main/0-Gas-Optimizations.md/#g007---long-revert-strings

In short, if you are using Solidity version >=0.8.4, the most gas-efficient approach is to create custom errors and use if (condition) revert customError() syntax. The second best approach is to shorten your error strings to have a length less than 32. Here’s where you can apply the shortening optimization in your error messages:

• https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/ExecutorManager.sol#L17

Although I’d recommend using custom errors for maximum savings.

Issue #4: Refactor for loops to be cheaper

Impact

For loops where you don’t think i will ever reach values of 2**256 can be optimized with the use of unchecked. You can also cache the length prior to executing the for loop, to avoid computing it every time.

Steps to Mitigate

Here’s an example of one of your for loops:

function addExecutors(address[] calldata executorArray) external override onlyOwner {
        for (uint256 i = 0; i < executorArray.length; ++i) {
            addExecutor(executorArray[i]);
        }
    }

It can be optimized by doing:

function addExecutors(address[] calldata executorArray) external override onlyOwner {
				uint256 executorArrayLength = executorArray.length;
        for (uint256 i; i < executorArrayLength;) {
						unchecked { i++;}
            addExecutor(executorArray[i]);
        }
    }

Tools Used

Used mini contract to test this using Foundry. Here are the results:

Optimized contract: 194 bytes of code, 25699 gas cost

Non-optimized contract: 223 byted of code, 27254 gas cost

Here are the contracts:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.10;

contract ForLoop {
    uint256 randomStateVariable;
    uint256[] randomArray = [1,2,3,4,5,6,7,8,9,10];

    function a() public {
        for (uint256 i = 0; i < randomArray.length; ++i) {
            randomStateVariable +=  i;
        }
    }
}
contract ForLoopOptimized {
    uint256 randomStateVariable;
    uint256[] randomArray = [1,2,3,4,5,6,7,8,9,10];

    function a() public {
        uint256 length = randomArray.length;
        for (uint256 i; i < length;) {
            unchecked {i++;}
            randomStateVariable +=  i;
        }
    }
}

And here the tests:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.10;

import "ds-test/test.sol";
import '../ForLoop.sol';

contract ForLoopTest is DSTest {
    ForLoop forLoop;
    ForLoopOptimized forLoopOptimized;
    function setUp() public {
        forLoop = new ForLoop();
        forLoopOptimized = new ForLoopOptimized();
    }

    function testForLoop() public {
        forLoop.a();
    }
    function testForLoopOptimized() public {
        forLoopOptimized.a();
    }
}

Issue #5: Variable packing inside structs

Impact

Savings can be made by packing multiple variables into a single slots. In LiquidityFarming there are three structs:

• NFTInfo: you can pack the bool and the address, but I didn’t find any meaningful save here.
• PoolInfo: lastRewardTime holds the block.timestamp, you can make this a uint32 valid up to 4294967296**,** which is Sunday, February 7, 2106 6:28:16 AM in human time. Or uint64, which is valid up to Sunday, July 21, 2554 11:33:20 PM. And make the other variable uint224 or uint192 if you consider the values stored will fit in these uints.
• RewardsPerSecondEntry: Same reasoning as PoolInfo

Issue #6: Non-necessary nonReentrant modifier

Impact

Save on gas and deployment costs by removing unnecessary nonReentrant modifiers. I’m pretty convinced these two are not necessary:

• https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityFarming.sol#L180
• https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L149

In the first case, no balances are updated after the external call, and I don’t see the function used anywhere else, so I believe it should be safe.

In the second case, the same applies, I don’t see a way to re-enter and mess with the balances or withdraw unlimited funds.

Issue #7: Move checks to the beginning of the function as possible

Impact

Save the user gas on reverts by eliminating any unnecessary computation happening before error checks here:

• https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityFarming.sol#L196

Steps to Mitigate

Refactor to:

function deposit(uint256 _nftId, address payable _to) external whenNotPaused nonReentrant {
    address msgSender = _msgSender();

    require(
        lpToken.isApprovedForAll(msgSender, address(this)) || lpToken.getApproved(_nftId) == address(this),
        "ERR__NOT_APPROVED"
    );

    (address baseToken, , uint256 amount) = lpToken.tokenMetadata(_nftId);
    require(rewardTokens[baseToken] != address(0), "ERR__POOL_NOT_INITIALIZED");
    require(rewardRateLog[baseToken].length != 0, "ERR__POOL_NOT_INITIALIZED");

    NFTInfo storage nft = nftInfo[_nftId];

    require(!nft.isStaked, "ERR__NFT_ALREADY_STAKED");

    lpToken.safeTransferFrom(msgSender, address(this), _nftId);

    amount /= liquidityProviders.BASE_DIVISOR();

    PoolInfo memory pool = updatePool(baseToken);
    nft.isStaked = true;
    nft.staker = _to;
    nft.rewardDebt = (amount * pool.accTokenPerShare) / ACC_TOKEN_PRECISION;

    nftIdsStaked[_to].push(_nftId);
    totalSharesStaked[baseToken] += amount;

    emit LogDeposit(msgSender, baseToken, _nftId);
}

Issue #8: Use unchecked in arithmetics operations that can’t over or underflow

Impact

Since solidity 0.8.0, the over/underflowing checks have been built into the evm. This wasn’t free. Solidity knows this and provides us with a way of cheapening the arithmetic operations we know won’t revert by using unchecked.

You can do this here:

• https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L179

Because in the if we previously check that currentLiquidity < providedLiquity, we know that liquidityDifference = providedLiquidity - currentLiquidity can ever over or underflow. So we can cheapen that using unchecked

Steps to Mitigate

Add unchecked to the liquidityDifference calculation:

unchecked {
    uint256 liquidityDifference = providedLiquidity - currentLiquidity;
}