Biconomy Hyphen 2.0 contest - benk10's results

Lines of code

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L233-L237

Vulnerability details

Impact

The impact of this exploit could be inability of user or users to withdraw any of their tokens from the HyphenLiquidityFarming contract, which would be critical. The likelihood of it to happen though, although possible, is extremely unlikely (unless we assume the owner address of the contract or a reward token is compromised or becomes malicious, in which case it is a very plausible scenario). Therefor I classify this issue at a medium risk.

Proof of Concept

In the LiquidityFarming.sol, the withdraw function includes an unbounded loop searching for the index of the NFT in the array of user staked NFTs. This loop, if too long, could cost more gas than the maximum allowed in a single transaction, which would make the call to it run out of gas, causing the function to fail and the tokens at the end of the nftIdsStaked array to be irredeemable until tokens at the beginning of the array are redeemed. However, if the reward token (the token rewarded according to the base token that is in the NFT metadata) of the NFTs in the beginning of the array becomes immovable for some reason (for example, being paused or malicious), these NFTs could become stuck (as the _sendRewardsForNft would fail on transferring the reward token, making potentially ALL the user tokens impossible to withdraw. This could happen accidentally by chance, but could becomes a full ransom attack vector in case a malicious token was registered (could happen for example if a reward token was upgradeable, then the ownership over the contract was compromised and the token replaced to be malicious, or by any other such malicious attack or accident in the owner of a token contract or the system itself). In such a case, the attacker could use the deposit function, which allows anyone to deposit tokens into any user address, to spam a user with nearly worthless NFTs until the array becomes to big for the user to withdraw the rest of their tokens. Then the user unsuspectingly deposit more tokens, which means they will be at the end of the nftIdsStaked of the user, irredeemable until the spam ones are redeemed. Then the attacker, assuming it controls the compromised rewards token, can pause transfers of the reward token, making ALL the users deposited NFTs impossible to withdraw (because the unbounded loop would be too long for the NFTs at the end of it, but the NFTs at the start would be impossible to remove). The attacker can then demand ransom from the victim to unpause the malicious token so the user could withdraw their NFTs again.

Recommended Mitigation Steps

In the withdraw function remove the loop and instead have index search done off chain, with only the correctness of the index being verified on chain. This could look as follows:

function withdraw(uint256 _nftId, address payable _to, unit256 index) external whenNotPaused nonReentrant {
        require(nftIdsStaked[msgSender][index] == _nftId, "index did not match");
        address msgSender = _msgSender();
...

Potential errors in executors list

Issue:

In the ExecutorManager.sol the function removeExecutor (line 53) does not remove the executor from the internal executors array (line 9) which could cause the function getAllExecutors (line 25) to return an incorrect array with addresses which are no longer executors.

Recommendation:

Delete the array entry of the removed executor on the removeExecutor function, or edit the getAllExecutors function to return only active executors by checking each address in the array against the executorStatus mapping and returning an array of only those with status true.

Change fee does not check token is supported

Issue:

In the TokenManager.sol the function changeFee (line 44) does not check that the token address is not 0 and is supported, which should be done using the tokenChecks modifier.

Recommendation:

Apply the tokenCheck modifier on the changeFee function in TokenManager.sol, as is done for example in the setTokenTransferOverhead function.

Wrong address in GasFeeWithdraw event for native token

Issue:

In the withdrawNativeGasFee function, the GasFeeWithdraw event is emitted with the LiquidityPool contract address as the native token address, instead of the hardcoded normal value of the NATIVE constant. This is both wrong and slightly more gas costly than the logical thing of emitting the NATIVE address as the token address.

Recommendation:

In the withdrawNativeGasFee function, emit the NATIVE address constant instead of the address of the LiquidityPool contract itself in the GasFeeWithdraw event, like this:

emit GasFeeWithdraw(NATIVE, _msgSender(), _gasFeeAccumulated);

Funds sent by mistake could be stuck in the contracts

Issue:

Multiple contracts, including the: LiquidityProviders.sol, LiquidityPool.sol, and LiquidityFarming.sol implement a receive function, which would allow the contract to accept any eth transaction sent to it. However, the contracts do not have a way to withdraw that extra ETH, and no reason to accept such ETH transfers that are not function calls in the first place. This could lead to loss of funds sent accidentally to the contracts, while adding no benefit and just making the contract size larger.

Recommendation:

Remove the above mentioned receive functions to avoid accidental loss of funds and reduce unnecessary contract code.

Use external visibility where appropriate

Issue:

Throughout the code, many functions that are declared as public could be declared as external. This includes:

• getExecutorStatus
• getAllExecutors
• getEquilibriumFee
• getMaxFee
• getTokensInfo
• getDepositConfig
• getTransferConfig
• initialize
• setTrustedForwarder
• setLiquidityProviders
• getExecutorManager
• initialize
• getTotalReserveByToken
• getSuppliedLiquidityByToken
• getTotalLPFeeByToken
• getCurrentLiquidity
• increaseCurrentLiquidity
• decreaseCurrentLiquidity
• getFeeAccumulatedOnNft
• initialize
• initialize
• setRewardPerSecond
• getNftIdsStaked
• initialize
• setSvgHelper
• getAllNftIdsByUser
• exists

Recommendation:

Declare these functions with external visibility.

Read from memory instead of storage when possible

Issue:

In TokenManager.sol line 53 a FeeChanged event is emitted, and both the equilibriumFee and maxFee parameters are being read from storage, while they already exist in memory as the function parameters _equilibriumFee and _maxFee.

Recommendation:

Save gas by emitting the event like so:

emit FeeChanged(tokenAddress, _equilibriumFee, _maxFee);

This will read the equilibriumFee and maxFee from storage instead of from memory which would save gas.

Avoid indexing unnecessary event parameters

Issue:

Indexing parameters makes events easily searchable by the indexed parameter, which for example is useeful to find all events related to a certain address. However, it increases gas costs for each indexed parameter, which is why it is not recommended to index parameters that do not have a reason to be the search criteria of an event, like transferred amounts. Throughout the code there are many event parameters which are indexed for no apparent reason, and it would save gas to not mark them as indexed. Like the following:

• FeeChanged: equilibriumFee, maxFee
• AssetSent: amount, transferredAmount
• FeeDetails: lpFee, transferFee, gasFee
• Received: amount
• GasFeeWithdraw: amount
• LiquidityAdded: amount
• LiquidityRemoved: amount
• FeeClaimed: fee
• FeeAdded: fee
• CurrentLiquidityChanged: oldValue, newValue

Recommendation:

Remove the indexed keyword from the above mentioned event parameters.

Make LiquidityPool.sol executorManager public

Issue:

LiquidityPool.sol declares executorManager as private, then adds a getter for it. It would be better to simply declare the executorManager as public, remove the getter, and use the solidity auto-generated getter of public varibales.

Recommendation:

Make executorManager public instead of private and remove the getter.

Avoid duplicated code in LiquidityPool deposit functions

Issue:

In LiquidityPool.sol the depositErc20 and depositNative functions are nearly identical, and could be united to a single deposit function which would reduce the contract size and adhere to the DRY (Don't Repeat Yourself) principle of coding.

Recommendation:

Remove the depositErc20 and depositNative functions and replace them with a single deposit function, which would look like this:

function deposit(
        uint256 toChainId,
        address tokenAddress,
        address receiver,
        uint256 amount,
        string memory tag
    ) public payable tokenChecks(tokenAddress) whenNotPaused nonReentrant {
        require(
            tokenManager.getDepositConfig(toChainId, tokenAddress).min <= amount &&
                tokenManager.getDepositConfig(toChainId, tokenAddress).max >= amount,
            "Deposit amount not in Cap limit"
        );
        require(receiver != address(0), "Receiver address cannot be 0");
        require(amount != 0, "Amount cannot be 0");
        address sender = _msgSender();

        uint256 rewardAmount = getRewardAmount(amount, tokenAddress);
        if (rewardAmount != 0) {
            incentivePool[tokenAddress] = incentivePool[tokenAddress] - rewardAmount;
        }
        liquidityProviders.increaseCurrentLiquidity(tokenAddress, amount);
        if (tokenAddress != NATIVE) {
            SafeERC20Upgradeable.safeTransferFrom(IERC20Upgradeable(tokenAddress), sender, address(this), amount);
        } else {
            require(amount == msg.value, "Wrong value");
        }
        // Emit (amount + reward amount) in event
        emit Deposit(sender, tokenAddress, receiver, toChainId, amount + rewardAmount, rewardAmount, tag);
    }

Avoid duplicated code in LiquidityPool withdrawGasFee functions

Issue:

In LiquidityPool.sol the withdrawErc20GasFee and withdrawNativeGasFee functions are nearly identical, and could be united into a single withdrawGasFee function which would reduce the contract size and adhere to the DRY (Don't Repeat Yourself) principle of coding.

Recommendation:

Remove the withdrawErc20GasFee and withdrawNativeGasFee functions and replace them with a single withdrawGasFee function, which would look like this:

function withdrawGasFee(address tokenAddress) external onlyExecutor whenNotPaused nonReentrant {
        // uint256 gasFeeAccumulated = gasFeeAccumulatedByToken[tokenAddress];
        uint256 _gasFeeAccumulated = gasFeeAccumulated[tokenAddress][_msgSender()];
        require(_gasFeeAccumulated != 0, "Gas Fee earned is 0");
        gasFeeAccumulatedByToken[tokenAddress] = gasFeeAccumulatedByToken[tokenAddress] - _gasFeeAccumulated;
        gasFeeAccumulated[tokenAddress][_msgSender()] = 0;
        if (tokenAddress != NATIVE) {
            SafeERC20Upgradeable.safeTransfer(IERC20Upgradeable(tokenAddress), _msgSender(), _gasFeeAccumulated);
        } else {
            (bool success, ) = payable(_msgSender()).call{value: _gasFeeAccumulated}("");
            require(success, "Native Transfer Failed");
        }
        emit GasFeeWithdraw(tokenAddress, _msgSender(), _gasFeeAccumulated);
    }

Avoid duplicated getters in LiquidityProviders.sol

Issue:

In LiquidityProviders.sol, the following functions are completely unnecessary:

• getTotalReserveByToken
• getSuppliedLiquidityByToken
• getTotalLPFeeByToken
• getCurrentLiquidity

That's because they are simple getters to variables that are declared as public, which means Solidity already generates for them a default getter which works just like the above listed functions, making the above listed functions a waste of code and contract size.

Recommendation:

Remove the above listed functions and instead use the default getters Solidity generates for all public variables.

Avoid unnecessary writing of lpFeeAccumulated to memory

Issue:

In LiquidityProviders.sol, the code in the getFeeAccumulatedOnNft could be gas-optimized by returning the result immediately instead of saving it to memory as the lpFeeAccumulated variable.

Recommendation:

Return the result immediately instead of saving it, by changing the function to be as follows:

function getFeeAccumulatedOnNft(uint256 _nftId) external view returns (uint256) {
        require(lpToken.exists(_nftId), "ERR__INVALID_NFT");

        (address _tokenAddress, uint256 nftSuppliedLiquidity, uint256 totalNFTShares) = lpToken.tokenMetadata(_nftId);

        if (totalNFTShares == 0) {
            return 0;
        }
        // Calculate rewards accumulated
        uint256 eligibleLiquidity = sharesToTokenAmount(totalNFTShares, _tokenAddress);

        // Handle edge cases where eligibleLiquidity is less than what was supplied by very small amount 
        if(nftSuppliedLiquidity > eligibleLiquidity) {
            return 0;
        } else {
            unchecked {
                return eligibleLiquidity - nftSuppliedLiquidity;
            }
        }
    }

Avoid unnecessary requires at addTokenLiquidity

Issue:

In LiquidityProviders.sol, the addTokenLiquidity function includes 2 require statements that are redundant, since if any of them is not met, the safeTransferFrom call right below will revert - both in case the token address is the NATIVE (as it doesn't exist), and in case there's no sufficient allowance.

Recommendation:

Remove the above mentioned require statements to save gas.

Move calculation of object index in array off chain

Issue:

In LiquidityFarming.sol, the withdraw function has a loop searching for the index of the token in the array of user tokens. This calculation could be moved off chain, and only be verified on chain for correctness to save gas on the list.

Recommendation:

Remove the loop and instead have the function as follows:

function withdraw(uint256 _nftId, address payable _to, unit256 index) external whenNotPaused nonReentrant {
        require(nftIdsStaked[msgSender][index] == _nftId, "index did not match");
        address msgSender = _msgSender();
...