Biconomy Hyphen 2.0 contest - 0xDjango's results

Issue #1

No 0 address check on Deposit. Wallet may default to 0 upon lack of input parameter causing permanent loss of NFT.

Link to Code

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

Steps to Fix

require(_to != address(0), "Address Can't Be Zero")

Extra Info

I'm submitting this bug as I was reviewing a similar submission in prior contest. https://github.com/code-423n4/2022-01-elasticswap-findings/issues/87#issuecomment-1030627836

Issue #2

Nft.upaidRewards reset upon withdrawal. This feels a bit unfair if a user means to collect rewards while withdrawing NFT. If there are not enough rewards in the contract balance, the unpaidRewards will be deleted upon withdrawal with the line delete nftInfo[_nftId];. I'm not sure the user will have visibility into the contract's current balance, whether Native or token, prior to deciding to withdraw. Either way, I think it would be a bad user experience to withdraw and realize all of your pending rewards for a potentially long staking period were erased due to bad timing.

Link to Code

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

Steps to Fix

Consider adding an emergencyWithdraw method which explicitly states that pending rewards might be forfeited or keeping track of users' unpaid balances somewhere other than nftInfo[_nftId] which is deleted upon withdrawal. Optional ability for users to claim unpaid rewards AFTER withdrawal following an increase in the contract's balance.

Issue

Repeated calls to tokenManager.getDepositConfig() in single require statement in LiquidityPool.sol. The same require statement is used in both the depositErc20 and despositNative functions.

Link to Code

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L156-L160

Suggested Change

TokenConfig memory tc = getDepositConfig(); 
require(tc.min <= amount && tc.max >= amount, "Deposit amount not in Cap limit");

Gas Optimization Testing

I tested both the original and revised methods in Remix and found that the revised method consumed 508 less gas than the original implementation. See code below:

// SPDX-License-Identifier: GPL-3.0

pragma solidity 0.8.0;

contract GasTest {

    struct TokenConfig {
        uint256 min;
        uint256 max;
    }

    mapping(uint256 => mapping(address => TokenConfig)) public depositConfig;

    function setDepositConfig() public {
        depositConfig[0][msg.sender].min = 1;
        depositConfig[0][msg.sender].max = 9;
    }

    function getDepositConfig() public view returns(TokenConfig memory) {
        return depositConfig[0][msg.sender];
    }

    // Consumes 26789 gas
    function gasTestOriginal(uint256 amount) public view {
        require(getDepositConfig().min <= amount && getDepositConfig().max >= amount, "Deposit amount not in Cap limit");
    }
    
    // Consumes 26281 gas
    function gasTestRevised(uint256 amount) public view {
        TokenConfig memory tc = getDepositConfig();
        require(tc.min <= amount && tc.max >= amount, "Deposit amount not in Cap limit");
    }
    
}