Biconomy Hyphen 2.0 contest - ye0lde's results

Biconomy QA Report

Unless otherwise noted, manual auditing and testing were done using Visual Studio Code and Remix. The audit was done from March 10 to March 16, 2022, by ye0lde through code4rena.

Overall, I found the code to be clear to follow and read.

• Findings
  • L-1 - Function deposit allows a _to address of 0 (LiquidityFarming.sol)

Findings

L-1 - Function deposit allows a _to address of 0 (LiquidityFarming.sol)

Impact

The deposit function sets nft.staker = _to and sets nftIdsStaked[_to].push(_nftId). When to is 0 this prevents withdraw and extractRewards from working correctly and the deposit is effectively lost without intervention from the protocol owner.

Proof of Concept

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

See @audit comments below.

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);
    amount /= liquidityProviders.BASE_DIVISOR();

    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);

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

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

    emit LogDeposit(msgSender, baseToken, _nftId);
}

Withdraws will be blocked at this line because msgSender will not be 0. https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L231

uint256 nftsStakedLength = nftIdsStaked[msgSender].length;

Extracting rewards will be blocked at this line because nftInfo[_nftId].staker will be 0. https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L259

require(nftInfo[_nftId].staker == _msgSender(), "ERR__NOT_OWNER");

Recommended Mitigation Steps

In function deposit require parameter _to to be non-zero.