Biconomy Hyphen 2.0 contest - kenta's results

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityFarming.sol#L140 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityFarming.sol#L145 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L251 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L336

Vulnerability details

Impact

There are some places in which the low-level function call is used. The internal function _sendRewardsForNft in LiquidityFarming.sol will be called by withdraw and extractRewards. The parameter of these functions _to will be the address of address.call, but they are not checked if they are empty or not. In addNativeLiquidity and increaseNativeLiquidity of LiquidityProvider.sol address.call is also used. In this case, the address for the call is address(liquidityPool). However, in setLiquidityPool, there is no validation check for parameter _liquidityPool.

There are some risks if they will be called with an empty address, because they will be not reverted.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityFarming.sol#L140 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityFarming.sol#L145

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L251 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L336

Tools Used

review

Recommended Mitigation Steps

check a parameter if this input is empty or not in withdraw, extractRewards in LiquidityFarming.sol and setLiquidityPool in LiquidityProvider.sol.

2022-03-biconomy

1 executorAddress must be deleted from executors in removeExecutor. Excutor will be added into executors in addExecutor. Input(excutorAddress) of removeExcutor must be deleted from executors in this function. Otherwise executors has the same address in the array if you will add the deleted address into executors again.

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

For example,

uint length = executors.length; uint index; for (uint i; i < length; ++i) { if (_name == executors[i]) { index = i; break; } } executors[index] = executors[length -1]; executors.pop();

2 delete unused variable name for return value

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/TokenManager.sol#L139

function _msgSender() internal view virtual override(Context, ERC2771Context) returns (address) { return ERC2771Context._msgSender(); }

3 missing input validation for _liquidityPool. The owner can change always liquidityPool but this liquidityPool will be used to execute low-level calls. To avoid errors with an empty address this must be checked always.

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L171-L173

require(_liquidityPool != address(0), “liquidityPool cannot be 0x0”);

4 wrong description for the following line.

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/WhitelistPeriodManager.sol#L26

For examples // Token -> LP Adress -> TVL

5 delete unused import statements.

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/WhitelistPeriodManager.sol#L6-L7

Delete them.

6 wrong description for setRewardPerSecond in HyphenLiquidityFarming. The following line must be wrong description. sushi?

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

7 Use the attached library. You have already attached SafeERC20Upgradeable for IERC20Upgradeable, so you change the following lines in LiquidityProviders.

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L273 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L325

IERC20Upgradeable(_token).safeTransferFrom(_msgSender(), address(liquidityPool), _amount);

IERC20Upgradeable(token).safeTransferFrom(_msgSender(), address(liquidityPool), _amount);

8 delete unused import statement.

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

9 visibility must be external. increaseCurrentLiquidity and decreaseCurrentLiquidity of LiquidityProvider.sol can be called only by LiquidityPool contract, so visibility must be external instead of public.

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L127 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L131

function increaseCurrentLiquidity(address tokenAddress, uint256 amount) external onlyLiquidityPool {

function decreaseCurrentLiquidity(address tokenAddress, uint256 amount) external onlyLiquidityPool {

2022-03-biconomy gas optimization

1 use initial value for uint256 in loop

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/ExecutorManager.sol#L31 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/ExecutorManager.sol#L47 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/TokenManager.sol#L78 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/LPToken.sol#L77 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/WhitelistPeriodManager.sol#L180 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/WhitelistPeriodManager.sol#L228 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityFarming.sol#L233

for (uint256 i; i < array.length; ++i) {}

Or much better if you write like the following

for (uint256 i; i < array.length;) { // some executions Unchecked {++1; } }

2 != is cheaper than >.

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L182 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L239 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L283 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L410 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityFarming.sol#L132 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityFarming.sol#L318

For example

if (supply != 0) {}

TokenManager.sol

3 use params instead of storage to emit event.

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/TokenManager.sol#L53

emit FeeChanged(tokenAddress, _equilibriumFee, _maxFee);

4 use cache for transferConfig[tokenAddress] in addSupportedToken.

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/token/TokenManager.sol#L93-L96

TokenConfig storage _tokenConfig = transferConfig[tokenAddress]; _tokenConfig.min = minCapLimit; _tokenConfig.max = maxCapLimit; tokensInfo[tokenAddress].tokenConfig = _tokenConfig; LiquidityPool.sol

5 use cache for tokenManager.getDepositConfig(toChainId, tokenAddress) in depositErc20 and depositNative

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L157-L158 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L248-L249 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L273-L274

For example,

ITokenManager.TokenConfig memory depositConfig = tokenManager.getDepositConfig(toChainId, tokenAddress); require( depositConfig.min <= amount && depositConfig.max >= amount, "Deposit amount not in Cap limit" );

6 the following lines must be checked earlier to save gas if they will be reverted. They must be placed at the beginning of the function.

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L161-L162 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L247-L253

For example

require(receiver != address(0), "Receiver address cannot be 0"); require(amount != 0, "Amount cannot be 0"); ITokenManager.TokenConfig memory depositConfig = tokenManager.getDepositConfig(toChainId, tokenAddress); require( depositConfig.min <= amount && depositConfig.max >= amount, "Deposit amount not in Cap limit" );

7 use unchecked because underflow is already checked before this calculation.

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

uint256 liquidityDifference; unchecked { providedLiquidity - currentLiquidity; }

8 gasLeft() must be called later and the following line must be checked earlier to save gas if they will be reverted. They must be placed at the beginning of the function.

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

require(receiver != address(0), "Bad receiver address"); can be placed at the beginning of the function. And gasLeft() can be executed right before initialGas will be called. e.g line 283.

9 use calldata instead of memory to save gas.

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/WhitelistPeriodManager.sol#L178 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/WhitelistPeriodManager.sol#L220-L222

function setIsExcludedAddressStatus(address[] calldata _addresses, bool[] calldata _status) external onlyOwner {

function setCaps( address[] calldata _tokens, uint256[] calldata _totalCaps, uint256[] calldata _perTokenWalletCaps ) external onlyOwner {

10 Attach library function to IERC20Upgradeable to save deployment gas cost.

The following lines use SafeERC20Upgradeable. You can save deployment gas cost if you attract this library at the beginning of the contract.

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L170 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L293 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L379 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L407

using SafeERC20Upgradeable for IERC20Upgradeable;

IERC20Upgradeable(tokenAddress).safeTransferFrom(sender, address(this), amount); IERC20Upgradeable(tokenAddress).safeTransfer(receiver, amountToTransfer);

IERC20Upgradeable(tokenAddress).safeTransfer(_msgSender(), _gasFeeAccumulated); baseToken.safeTransfer(receiver, _tokenAmount);

11 += is cheaper in the followings lines.

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L331-L332 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityPool.sol#L335-L336

For example, totalGasUsed += tokenManager.getTokensInfo(tokenAddress).transferOverhead; totalGasUsed += baseGas;

gasFeeAccumulatedByToken[tokenAddress] += gasFee; gasFeeAccumulated[tokenAddress][_msgSender()] += gasFee;

12 -= is cheaper in the following lines,

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

gasFeeAccumulatedByToken[tokenAddress] -= _gasFeeAccumulated; gasFeeAccumulatedByToken[NATIVE] -= _gasFeeAccumulated;

13 Use unchecked for the following line. rewardAmount is calculated in getRewardAmount. rewardAmount will be never more than incentivePool[tokenAddress], so you can use unchecked for the following line.

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

if (rewardAmount != 0) { unchecked { incentivePool[tokenAddress] -= rewardAmount; } }

LiquidityProviders.sol

14 Delete unused variable. In onlyValidLpToken lpToken.tokenMetadata() is called with tokenId, but the return value token will be not used in modifier.

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L54

15 Use cache for previous state of state variable for currentLiquidity[tokenAddress] in _increaseCurrentLiquidity and _decreaseCurrentLiquidity. You can avoid calling sload one time.

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L136-L137 https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L141-L142

uint previousLiquidity = currentLiquidity[tokenAddress]; currentLiquidity[tokenAddress] = previousLiquidity + amount; emit CurrentLiquidityChanged(tokenAddress, previousLiquidity, previousLiquidity + amount);

uint previousLiquidity = currentLiquidity[tokenAddress]; currentLiquidity[tokenAddress] = previousLiquidity - amount; emit CurrentLiquidityChanged(tokenAddress, previousLiquidity, previousLiquidity - amount);

16 Use cached variable in getTokenPriceInLPShares. In getTokenPriceInLPShares there is a cached variable supply. You can use it in calculaton too.

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L181-L183

if (supply > 0) { return supply / totalReserve[_baseToken]; }

17 The following line must be place at the beginning of function to save gas if it reverts the excution.

https://github.com/code-423n4/2022-03-biconomy/blob/main/contracts/hyphen/LiquidityProviders.sol#L283

LiquidityFarming.sol

18 Use cache for rewardRateLog[_baseToken] in getRewardRatePerSecond

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

function getRewardRatePerSecond(address _baseToken) public view returns (uint256) { RewardsPerSecondEntry[] memory rewardsPerSecondEntry = rewardRateLog[_baseToken]; return rewardsPerSecondEntry[rewardsPerSecondEntry.length - 1].rewardsPerSecond; }

19 Use cache for nftIdsStaked[msgSender] in withdraw.

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

uint256[] storage _nftsStaked = nftIdsStaked[msgSender]; uint256 index; for (index = 0; index < _nftsStaked.length; ++index) { if (_nftsStaked[index] == _nftId) { break; } }

    require(index != _nftsStaked.length, "ERR__NFT_NOT_STAKED");
    _nftsStaked[index] = _nftsStaked[_nftsStaked.length - 1];
    _nftsStaked.pop();

20 Use cache for poolInfo[_baseToken] in getUpdatedAccTokenPerShare

The following lines use sload for poolInfo[_baseToken]. You can save gas with memory cache.

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

PoolInfo memory pool = poolInfo[_baseToken]; uint256 lastUpdatedTime = pool.lastRewardTime;

return accumulator + pool.accTokenPerShare;

21 Use cache for rewardRateLog[_baseToken]in getUpdatedAccTokenPerShare

In the following lines you can use memory cache for rewardRateLog[_baseToken]

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

RewardsPerSecondEntry[] memory _rewardsPerSecondEntry = rewardRateLog[_baseToken]; uint256 i = _rewardsPerSecondEntry.length - 1; while (true) { if (lastUpdatedTime >= counter) { break; } unchecked { accumulator += _rewardsPerSecondEntry[i].rewardsPerSecond * (counter - max(lastUpdatedTime, _rewardsPerSecondEntry[i].timestamp)); } counter = _rewardsPerSecondEntry[i].timestamp;

22 Use cache for poolInfo[_baseToken] and totalSharesStaked[_baseToken] in updatePool. You can cache poolInfo[_baseToken] as storage and totalSharesStaked[_baseToken] as memory to save gas cost in this function.

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

function updatePool(address _baseToken) public whenNotPaused returns (PoolInfo memory) { PoolInfo storage pool = poolInfo[_baseToken]; uint256 _totalSharesStaked = totalSharesStaked[_baseToken]; if (block.timestamp > pool.lastRewardTime) { if (_totalSharesStaked > 0) { pool.accTokenPerShare = getUpdatedAccTokenPerShare(_baseToken); } pool.lastRewardTime = block.timestamp; emit LogUpdatePool(_baseToken, pool.lastRewardTime, _totalSharesStaked, pool.accTokenPerShare); } return pool; }