Platform: Code4rena
Start Date: 10/03/2022
Pot Size: $75,000 USDT
Total HM: 25
Participants: 54
Period: 7 days
Judge: pauliax
Total Solo HM: 10
Id: 97
League: ETH
Rank: 38/54
Findings: 2
Award: $182.94
🌟 Selected for report: 1
🚀 Solo Findings: 0
https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/TokenManager.sol
The scope contracts do not appear to support rebasing/deflationary/inflationary tokens whose balance changes during transfers or over time. The necessary checks include at least verifying the amount of tokens transferred to contracts before and after the actual transfer to infer any fees/interest.
https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/TokenManager.sol
Make sure token vault accounts for any rebasing/inflation/deflation Add support in contracts for such tokens before accepting user-supplied tokens
#0 - ankurdubey521
2022-03-30T15:47:00Z
Duplicate of #39
#1 - pauliax
2022-04-26T10:49:14Z
Grouping all the issues related to the incompatibility with weird ERC20s together and making this a primary issue because it is the most generic.
🌟 Selected for report: Dravee
Also found by: 0v3rf10w, 0x1f8b, 0xDjango, 0xNazgul, 0xngndev, 0xwags, Cantor_Dust, CertoraInc, IllIllI, Jujic, Kenshin, Kiep, PPrieditis, TerrierLover, Tomio, WatchPug, antonttc, benk10, berndartmueller, bitbopper, csanuragjain, defsec, gzeon, hagrid, hickuphh3, kenta, minhquanym, oyc_109, pedroais, peritoflores, rfa, robee, saian, samruna, sirhashalot, throttle, wuwe1, z3s
83.6804 USDT - $83.68
The local variable used as for loop index need not be initialized to 0 because the default value is 0. Avoiding this anti-pattern can save a few opcodes and therefore a tiny bit of gas.
for (uint256 index = 0; index < tokenConfig.length; ++index) {
The same situation are in other scope contracts where loops use.
Remix
Remove explicit 0 initialization of for loop index variable.
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.
There are several other places throughout the codebase where the same optimization can be used.
Shorten the revert strings to fit in 32 bytes.
!= 0 is a cheaper operation compared to > 0, when dealing with uint.
https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L132 https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityProviders.sol#L182
There are several other places throughout the codebase where the same optimization can be used.
In the LiquidityPool.sol, declaring the type bytes32 can save gas.
https://medium.com/layerx/how-to-reduce-gas-cost-in-solidity-f2e5321e0395#2a78
NFTInfo struct i can be optimized to reduce 1 storage slot
struct NFTInfo { address payable staker; uint256 rewardDebt; uint256 unpaidRewards; bool isStaked; }
Moving the bool isStaked close to address payable staker it's possible to save one slot.
struct NFTInfo { address payable staker; bool isStaked; uint256 rewardDebt; uint256 unpaidRewards; }
For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.
if (currentLiquidity < providedLiquidity) { uint256 liquidityDifference = providedLiquidity - currentLiquidity;
uint256 tokenId = totalSupply() + 1;
uint256 i = rewardRateLog[_baseToken].length - 1;
// Per Token Total Cap or PTTC require(ifEnabled(totalLiquidity[_token] + _amount <= perTokenTotalCap[_token]), "ERR__LIQUIDITY_EXCEEDS_PTTC"); require( ifEnabled(totalLiquidityByLp[_token][_lp] + _amount <= perTokenWalletCap[_token]), "ERR__LIQUIDITY_EXCEEDS_PTWC" ); totalLiquidity[_token] += _amount; totalLiquidityByLp[_token][_lp] += _amount;
Consider using 'unchecked' where it is safe to do so.
Some of the variable can be cached to slightly reduce gas usage.
lpToken can be cached.
https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L196-L213
tokenManager can be cached.
https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L308-L340
Remix
Consider caching those variable for read and make sure write back to storage
count is being assigned its default value.The constant variable is being assigned its default value which is unnecessary.
uint256 count = 0;
Remove the assignment.
Custom errors are cheaper than revert strings.
Source: https://blog.soliditylang.org/2021/04/21/custom-errors/: Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them. Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).
Replace revert strings with custom errors.
More expensive gas usage
require( tokenManager.getDepositConfig(toChainId, tokenAddress).min <= amount && tokenManager.getDepositConfig(toChainId, tokenAddress).max >= amount, "Deposit amount not in Cap limit" );
Instead of using operator && on single require check using double require check can save more gas
10 ** 18 for constantMore expensive gas usage
uint256 public constant BASE_DIVISOR = 10**18;
Change to:
uint256 public constant BASE_DIVISOR = 1e18;
Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free. The advantages of versions 0.8.* over <0.8.0 are: • Safemath by default from 0.8.0 (can be more gas efficient than library based safemath.) • Low level inliner : from 0.8.2, leads to cheaper runtime gas. Especially relevant when the contract has small functions. For example, OpenZeppelin libraries typically have a lot of small helper functions and if they are not inlined, they cost an additional 20 to 40 gas because of 2 extra jump instructions and additional stack operations needed for function calls. • Optimizer improvements in packed structs: Before 0.8.3, storing packed structs, in some cases used an additional storage read operation. After EIP-2929, if the slot was already cold, this means unnecessary stack operations and extra deploy time costs. However, if the slot was already warm, this means additional cost of 100 gas alongside the same unnecessary stack operations and extra deploy time costs. • Custom errors from 0.8.4, leads to cheaper deploy time cost and run time cost. Note: the run time cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors. https://github.com/code-423n4/2021-10-mochi-findings/issues/34
Consider to upgrade pragma to at least 0.8.4.
Lower than uint256 size storage instance variables are actually less gas efficient. E.g. using uint128 does not give any efficiency, actually, it is the opposite as EVM operates on default of 256-bit values so uint128 is more expensive in this case as it needs a conversion. It only gives improvements in cases where you can pack variables together, e.g. structs.
function setBaseGas(uint128 gas) external onlyOwner { baseGas = gas; }
Consider to review all uint types. Change them with uint256 If the integer is not necessary to present with uint128.
Booleans are more expensive than uint256 or any type that takes up a full word because each write operation emits an extra SLOAD to first read the slot's contents, replace the bits taken up by the boolean, and then write back. This is the compiler's defense against contract upgrades and pointer aliasing, and it cannot be disabled. See: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol#L23-L27
mapping(bytes32 => bool) public processedHash;
// LP Fee Distribution mapping(address => uint256) public totalReserve; // Include Liquidity + Fee accumulated mapping(address => uint256) public totalLiquidity; // Include Liquidity only mapping(address => uint256) public currentLiquidity; // Include current liquidity, updated on every in and out transfer mapping(address => uint256) public totalLPFees; mapping(address => uint256) public totalSharesMinted;
The require statements in LiquidityPool.sol can be placed earlier to reduce gas usage.
https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L271
... uint256 initialGas = gasleft(); require( tokenManager.getTransferConfig(tokenAddress).min <= amount && tokenManager.getTransferConfig(tokenAddress).max >= amount, "Withdraw amnt not in Cap limits" ); require(receiver != address(0), "Bad receiver address"); ...
Relocate the said require statement
Contract SvgHelperBase.sol not use the import base64.sol library.
Consider reviewing all the unused imports and removing them to reduce the size of the contract and thus save some deployment gas.
Checking if _amont > 0 before making the external call can save gas. https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L109-L117
function _sendErc20AndGetSentAmount( IERC20Upgradeable _token, uint256 _amount, address _to ) private returns (uint256) { uint256 recepientBalance = _token.balanceOf(_to); _token.safeTransfer(_to, _amount); return _token.balanceOf(_to) - recepientBalance; }
Where possible, use equivalent function parameters or local variables in event emits instead of state variables to prevent expensive SLOADs. Post-Berlin, SLOADs on state variables accessed first-time in a transaction increased from 800 gas to 2100, which is a 2.5x increase.
The LogUpdatePool event in the updatePool() uses state variable totalSharesStaked[_baseToken] instead of using the caching variable.
function updatePool(address _baseToken) public whenNotPaused returns (PoolInfo memory pool) { pool = poolInfo[_baseToken]; if (block.timestamp > pool.lastRewardTime) { if (totalSharesStaked[_baseToken] > 0) { pool.accTokenPerShare = getUpdatedAccTokenPerShare(_baseToken); } pool.lastRewardTime = block.timestamp; poolInfo[_baseToken] = pool; emit LogUpdatePool(_baseToken, pool.lastRewardTime, totalSharesStaked[_baseToken], pool.accTokenPerShare); } }
The WhitelistPeriodManager.setIsExcludedAddressStatus function iterates over all addresses to check for status of an element in the _addresses array.
A more efficient solution would be to use (OpenZeppelin's EnumerableSet) https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/structs/EnumerableSet.sol
On external functions, when using the memory keyword with a function argument, what's happening is that a memory acts as an intermediate. Reading directly from calldata using calldataload instead of going via memory saves the gas from the intermediate memory operations that carry the values. As an extract from https://ethereum.stackexchange.com/questions/74442/when-should-i-use-calldata-and-when-should-i-use-memory : «memory and calldata (as well as storage) are keywords that define the data area where a variable is stored. To answer your question directly, memory should be used when declaring variables (both function parameters as well as inside the logic of a function) that you want stored in memory (temporary), and calldata must be used when declaring an external function's dynamic parameters. The easiest way to think about the difference is that calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.»
Use calldata instead of memory for external functions where the function argument is read-only.