search

code-423n4 / 2022-03-biconomy-findings

0 stars 0 forks source link

QA Report #143

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Upgrade Solidity Version

Recommended to upgrade to latest Solidity 0.8.12

Upgrade ERC2771Context to latest OZ implementation

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/metatx/ERC2771Context.sol to https://github.com/OpenZeppelin/openzeppelin-contracts/blob/52eeebecda140ebaf4ec8752ed119d8288287fac/contracts/metatx/ERC2771Context.sol

Upgrade ERC2771ContextUpgradeable to latest OZ implementation

https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/metatx/ERC2771ContextUpgradeable.sol to https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/3dec82093ea4a490d63aab3e925fed4f692909e8/contracts/metatx/ERC2771ContextUpgradeable.sol

No cap on fee parameters

Consider adding caps to fee parameters to reduce rug risk https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/token/TokenManager.sol#L44

Extra space

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

            " ERR_ARRAY_LENGTH_MISMATCH"

Make constants public

Consider make them public for easier external consumption https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityFarming.sol#L68 https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/LiquidityPool.sol#L19-20

removeExecutor lack check to see if the address is registered

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

Use BASE_DIVISOR constant instead of 10000000000

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

increaseNativeLiquidity lack msg.value != 0 check

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

Consider remove setLpToken function

Owner can call setLpToken to change the value of lpToken in WhitelistPeriodManager, which will make all onlyLpNft function revert https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/WhitelistPeriodManager#L170-176

    function _setLpToken(address _lpToken) internal {
        lpToken = ILPToken(_lpToken);
    }

    function setLpToken(address _lpToken) external onlyOwner {
        _setLpToken(_lpToken);
    }

Consider remove setLiquidityProviders function

Owner can call setLpToken to change the value of lpToken in WhitelistPeriodManager, which will make all onlyLiquidityPool function revert https://github.com/code-423n4/2022-03-biconomy/blob/04751283f85c9fc94fb644ff2b489ec339cd9ffc/contracts/hyphen/WhitelistPeriodManager#L162-168

    function _setLiquidityProviders(address _liquidityProviders) internal {
        liquidityProviders = ILiquidityProviders(_liquidityProviders);
    }

    function setLiquidityProviders(address _liquidityProviders) external onlyOwner {
        _setLiquidityProviders(_liquidityProviders);
    }
pauliax commented 2 years ago

"No cap on fee parameters" should be upgraded to Medium: https://github.com/code-423n4/2022-03-biconomy-findings/issues/8#issuecomment-1114023886

pauliax commented 2 years ago

"Consider remove setLpToken function" is similar to: #51

pauliax commented 2 years ago

"Consider remove setLiquidityProviders function" is similar to #52