search

code-423n4 / 2022-06-connext-findings

1 stars 0 forks source link

QA Report #120

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Init frontrun

Most contracts use an init pattern (instead of a constructor) to initialize contract parameters. Unless these are enforced to be atomic with contact deployment via deployment script or factory contracts, they are susceptible to front-running race conditions where an attacker/griefer can front-run (cannot access control because admin roles are not initialized) to initially with their own (malicious) parameters upon detecting (if an event is emitted) which the contract deployer has to redeploy wasting gas and risking other transactions from interacting with the attacker-initialized contract.

Many init functions do not have an explicit event emission which makes monitoring such scenarios harder. All of them have re-init checks; while many are explicit some (those in auction contracts) have implicit reinit checks in initAccessControls() which is better if converted to an explicit check in the main init function itself. (details credit to: https://github.com/code-423n4/2021-09-sushimiso-findings/issues/64) The vulnerable initialization functions in the codebase are:

Code instances:

    Replica.sol, initialize, 104
    PromiseRouter.sol, initialize, 146
    NomadBase.sol, __NomadBase_initialize, 97
    BridgeToken.sol, initialize, 34
    LPToken.sol, initialize, 21
    GovernanceRouter.sol, initialize, 179

Duplicates in array

    You allow in some arrays to have duplicates. Sometimes you assumes there are no duplicates in the array.

Code instance:

GovernanceRouter._addDomain pushed (_domain) and doesn't check if the domain already exists.

Mult instead div in compares

To improve algorithm precision instead using division in comparison use multiplication in the following scenario:

    Instead a < b / c use a * c < b.

In all of the big and trusted contracts this rule is maintained.

Code instances:

    RoutersFacet.sol, 349, if (_numerator < (denominator * 95) / 100) revert RoutersFacet__setLiquidityFeeNumerator_tooSmall();

Missing fee parameter validation

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

Code instances:

    StableSwap.setSwapFee (newSwapFee)
    StableSwap.setAdminFee (newAdminFee)
    SwapUtils.setSwapFee (newSwapFee)
    SwapUtils.setAdminFee (newAdminFee)
    StableSwapFacet.setSwapAdminFee (newAdminFee)
    StableSwapFacet.setSwapFee (newSwapFee)

Does not validate the input fee parameter

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

Code instances:

    StableSwap.initialize (_adminFee)
    PortalFacet.repayAavePortal (_feeAmount)
    StableSwap.initialize (_fee)
    SponsorVault.reimburseLiquidityFees (_liquidityFee)
    SwapUtils._feePerToken (swapFee)
    PortalFacet._backLoan (_fee)
    PortalFacet.repayAavePortalFor (_feeAmount)
    BridgeFacet._getFastTransferAmount (_liquidityFeeDen)
    StableSwapFacet.initializeSwap (_adminFee)
    StableSwapFacet.initializeSwap (_fee)

safeApprove of openZeppelin is deprecated

You use safeApprove of openZeppelin although it's deprecated. (see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/566a774222707e424896c0c390a84dc3c13bdcb2/contracts/token/ERC20/utils/SafeERC20.sol#L38) You should change it to increase/decrease Allowance as OpenZeppilin says.

Code instances:

    Deprecated safeApprove in RoutersFacet.sol line 267: if (s.routerPermissionInfo.approvedRouters[router]) revert RoutersFacet__setupRouter_alreadyAdded();
    Deprecated safeApprove in RoutersFacet.sol line 378: s.routerPermissionInfo.approvedForPortalRouters[_router] = false;

Require with empty message

The following requires are with empty messages. This is very important to add a message for any require. So the user has enough information to know the reason of failure.

Code instances:

    Solidity file: ExcessivelySafeCall.sol, In line 127 with Empty Require message.
    Solidity file: Multicall.sol, In line 18 with Empty Require message.

Not verified input

external / public functions parameters should be validated to make sure the address is not 0. Otherwise if not given the right input it can mistakenly lead to loss of user funds.

Code instances:

    SponsorVault.sol.withdraw _receiver
    GovernanceRouter.sol.initialize _recoveryManager
    DiamondCutFacet.sol.diamondCut _init
    XAppConnectionManager.sol.setHome _home

Solidity compiler versions mismatch

The project is compiled with different versions of solidity, which is not recommended because it can lead to undefined behaviors.

Code instance:

Not verified owner

    owner param should be validated to make sure the owner address is not address(0).
    Otherwise if not given the right input all only owner accessible functions will be unaccessible.

Code instances:

    RoutersFacet.sol.setupRouter owner
    BridgeToken.sol.transferOwnership _newOwner
    BridgeToken.sol.permit _owner

Named return issue

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.

Code instances:

    StableSwapFacet.sol, calculateRemoveSwapLiquidityOneToken
    TypedMemView.sol, indexLEUint
    TypedMemView.sol, index
    TypedMemView.sol, nibbleHex
    TypedMemView.sol, build
    TypedMemView.sol, hash160
    TypedMemView.sol, isValid
    StableSwap.sol, calculateRemoveLiquidityOneToken
    TypedMemView.sol, indexUint

Two Steps Verification before Transferring Ownership

The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked. It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership. A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105

Code instances:

    StableSwap.sol
    XAppConnectionClient.sol
    BaseConnextFacet.sol
    Home.sol
    GovernanceMessage.sol
    StableSwapFacet.sol
    BridgeToken.sol
    SwapUtils.sol
    GovernanceRouter.sol
    ConnextPriceOracle.sol
    IBridgeToken.sol
    NomadFacet.sol

Use safe math for solidity version <8

You should use safe math for solidity version <8 since there is no default over/under flow check it suchversions of solidity.

Code instances:

    The contract UpgradeBeaconProxy.sol doesn't use safe math and is of solidity version < 8
    The contract IMessageRecipient.sol doesn't use safe math and is of solidity version < 8
    The contract XAppConnectionClient.sol doesn't use safe math and is of solidity version < 8
    The contract UpgradeBeaconController.sol doesn't use safe math and is of solidity version < 8
jakekidd commented 2 years ago

Missing fee parameter validation and Does not validate the input fee parameter are the same

Some of the "code instances" above seem mismatched??

    The contract UpgradeBeaconProxy.sol doesn't use safe math and is of solidity version < 8
    The contract IMessageRecipient.sol doesn't use safe math and is of solidity version < 8
    The contract XAppConnectionClient.sol doesn't use safe math and is of solidity version < 8
    The contract UpgradeBeaconController.sol doesn't use safe math and is of solidity version < 8

all of these are out of scope

    Solidity file: ExcessivelySafeCall.sol, In line 127 with Empty Require message.
    Solidity file: Multicall.sol, In line 18 with Empty Require message.

out of scope