Connext Amarok contest - shenwilly's results

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/LibDiamond.sol#L95-L118

Vulnerability details

Malicious or compromised governance can send a different _init and _calldata payload than the one that was proposed. This is dangerous as they will be used for a delegatecall operation. An attacker can pretend to propose a safe upgrade and later execute a delegatecall to steal funds or selfdestruct the system.

Proof of Concept

• Malicious governor proposed a normal update with empty payload for _init and _calldata.
• After the update delay has passed, the governor executes the update with _init pointing to a malicious contract, stealing funds from the system.

Recommended Mitigation Steps

Consider hashing _init and _calldata alongside _diamondCut as the acceptanceTimes key, to ensure the execution will use the same payload.

// @audit Apply to rescindDiamondCut() and diamondCut() as well.
function proposeDiamondCut(
    IDiamondCut.FacetCut[] memory _diamondCut,
    address _init,
    bytes memory _calldata
) internal {
    uint256 acceptance = block.timestamp + _delay;
    diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut, _init, _calldata))] = acceptance;
    emit DiamondCutProposed(_diamondCut, _init, _calldata, acceptance);
} 

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/libraries/LibDiamond.sol#L100-L103

Vulnerability details

The current implementation of LibDiamond.diamondCut allows any diamondCut update to be executed instantly, defeating the purpose of the 7 day update delay mechanism.

The issue is this check in LibDiamond.diamondCut:

require(
  diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut))] < block.timestamp,
  "LibDiamond: delay not elapsed"
);

As acceptanceTimes has the default value of 0, any updates that have never been proposed will satisfy the < block.timestamp condition. A malicious or compromised governance can exploit this to add a malicious upgrade instantly.

Recommended Mitigation Steps

Add another check to guard against this:

require(
    diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut))] != 0,
    "LibDiamond: not proposed yet"
);

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/BridgeFacet.sol#L827-L850 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/SponsorVault.sol#L138-L140

Vulnerability details

When a sponsor wants to stop sponsoring bridge fees, it's possible that they call SponsorVault.setConnext to stop allowing Connext to reimburse fees, but forgot to inform the Connext team. This will cause bridging executions to be temporarily disrupted.

For fee reimbursement to work, both the Bridge and SponsorVault must point to each other. If SponsorVault's connext is set to address zero but Connext bridge's sponsorVault is still set to the vault, any bridge transaction to this chain will revert as the bridge still try to reimburse the fees from the sponsor's vault:

function _handleExecuteTransaction() {
    ...
    if (address(s.sponsorVault) != address(0)) {
        ...       
       // reverts here
       s.sponsorVault.reimburseRelayerFees(_args.params.originDomain, payable(_args.params.to), _args.params.relayerFee);
    }
    ...
}

Proof of Concept

• Chain A sponsor decided to stop sponsoring, calls setConnext to address zero.
• Every bridging transactions to Chain A stops being executed.
• Connex team realises after a period of time and sets sponsorVault to address zero so that transactions start being executed again.

Recommended Mitigation Steps

Add a function to deactivate sponsorVault atomically when sponsorVault calls setConnext() to another address.

In BridgeFacet.sol:

function deactivateSponsorVault() external {
    if (msg.sender != address(s.sponsorVault)) revert BridgeFacet__notSponsorVault;
    
    s.sponsorVault = ISponsorVault(address(0));
    emit SponsorVaultUpdated(msg.sender, address(0), msg.sender);
}

In SponsorVault.sol:

function setConnext(address _connext) external onlyOwner {
    if (IConnextHandler(connext).sponsorVault() == address(this)) {
        IConnextHandler(connext).deactivateSponsorVault();    
    }   
    _setConnext(_connext);
}

Low Risk Vulnerabilities

1. Unnecessary sponsoredFee calculation

When calling SponsorVault.reimburseRelayerFees with active gasTokenOracle, sponsoredFee is calculated after getting the gas rate from oracle. This is unnecessary as sponsoredFee will be calculated again later in the code.

In addition, there's no zero division check so it is possible this issue could cause a revert in a proper transaction.

Recommended Mitigation Steps

Remove the unnecessary line of code.

Related codes

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/SponsorVault.sol#L246

2. Query ERC20 decimals directly on-chain

When initializing stable swap pools, decimals are provided manually by the deployer. A faulty script could insert the wrong decimal values and make the pool unusable, requiring a redeployment.

Recommended Mitigation Steps

Remove decimals parameter and query token decimals directly on-chain.

uint8 decimals = IERC20Extended(_pooledTokens[i]).decimals()
precisionMultipliers[i] = 10**uint256(SwapUtils.POOL_PRECISION_DECIMALS - decimals);

Related codes

https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/helpers/StableSwap.sol#L61-L91 https://github.com/code-423n4/2022-06-connext/blob/4dd6149748b635f95460d4c3924c7e3fb6716967/contracts/contracts/core/connext/facets/StableSwapFacet.sol#L393-L426

Non-Critical Vulnerabilities

1. Typos

1. L137: _potentialReplcia should be _potentialReplica.
2. L54: _potentialReplcia should be _potentialReplica.
3. L575: specicfied should be specified.
4. L165: no enough asset should be not enough asset.
5. L826 if sponsored should be is sponsored.