Connext Amarok contest - csanuragjain's results

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/facets/BridgeFacet.sol#L279

Vulnerability details

Impact

A approved realyer can steal from sponsor vault by self initiating transaction via xcall with little to no relayer fees. Then Relayer can himself execute the transaction and claim the fees which he gave and also from the sponsor vault. Thus stealing from sponsor vault

Proof of Concept

1. Relayer makes xcall using destination domain as his own or some other where he is the approved relayer and also have some sponsor vault

2. Since relayer fees on this transaction is very low so no relayer would be interested in taking this transaction.

3. The malicious Relayer simply execute his own transaction due to non competetion and also in process obtains the fees from sponsor vault for his own transaction

Recommended Mitigation Steps

There should be a min cap for relayer fees

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/LibDiamond.sol#L83

Vulnerability details

Impact

The rescindDiamondCut function will set acceptanceTimes of _diamondCut to 0 which is incorrect. This will simply bypass the delay at LibDiamond.sol#L101

Proof of Concept

1. Admin calls proposeDiamondCut to set a new proposal

uint256 acceptance = block.timestamp + _delay;
    diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut))] = acceptance;

2. Ideally Admin has to wait till block.timestamp + _delay to execute this Diamond cut

3. But this can be bypaased

4. Admin can simply call rescindDiamondCut function which sets

diamondStorage().acceptanceTimes[keccak256(abi.encode(_diamondCut))] = 0;

5. Now if Admin calls diamondCut then delay gets bypassed since 0<block.timestamp is true

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

Recommended Mitigation Steps

Revise the condition at LibDiamond.sol#L100 to

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

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/facets/BridgeFacet.sol#L839

Vulnerability details

Impact

In case the asset id used in the xcall takes transfer fees while transferring then execution will fail causing DOS

Proof of Concept

1. User A makes an xcall with an asset id taking transfer fees

2. Relayer executes this transaction

3. s.sponsorVault.reimburseLiquidityFees at BridgeFacet.sol#L836 will initiate reimburseLiquidityFees function on SponsorVault.sol#L196

4. IERC20(_token).safeTransfer(msg.sender, sponsoredFee); will transfer the sponsoredFee to BridgeFacet.sol

5. Since _token has a transfer fees so amount transferred to BridgeFacet.sol will be sponsoredFee-delta

6. This becomes a problem at BridgeFacet.sol#L839 which will fail since (IERC20(_asset).balanceOf(address(this)) != starting + sponsored) because of the transfer fees

7. Hence fast execution will not be possible on this transaction

Recommended Mitigation Steps

Sponsored vault should return the actual amount which is transferred in the sponsored variable

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/facets/BridgeFacet.sol#L411

Vulnerability details

Impact

Relayer and Router can combine to steal funds by taking over most incoming transactions

Proof of Concept

1. Relayer checks the mempool for any user who has called xcall function

2. Once relayer gets a transaction with heavy amount, relayer can note all args passed in the xcall by the user

3. Now assuming both Relayer and Router are working together on this fraud, Relayer already has the Router signature

4. Relayer can simply use execute function to complete this transaction using the args obtained in frontrun which will result in same transaction id.

5. This bypasses the Sequencer bid and give unfair advantage to this Relayer

6. Relayer can gain all the fees now once user transaction arrives via nomad and can share his fees profit between himself and router. All other relayers will be disadvantaged

Recommended Mitigation Steps

Keep a tab on total number of transactions relayer is able to execute. If only certain relayer are executing most transactions, probably check if above issue is not happening

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/facets/StableSwapFacet.sol#L279 https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/facets/StableSwapFacet.sol#L305

Vulnerability details

Impact

Even when contract is in pause state both swapExactOut and addSwapLiquidity can still be called due to missing modifier check whenNotPaused

Proof of Concept

1. Observe the swapExactOut function

2. Observe that the function is missing whenNotPaused modifier

3. This means that swapping will be possible even when Admin has kept the contract in paused state (due to some emergency). This is incorrect behaviour

Recommended Mitigation Steps

Add modifier whenNotPaused for both swapExactOut and addSwapLiquidity function

function swapExactOut(
    bytes32 canonicalId,
    uint256 amountOut,
    address assetIn,
    address assetOut,
    uint256 maxAmountIn,
    uint256 deadline
  ) external payable nonReentrant deadlineCheck(deadline) whenNotPaused returns (uint256)

Oracle returns Chainlink latestRoundData without proper validation

Lines of code

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/helpers/ConnextPriceOracle.sol#L122

Vulnerability details

Impact

Oracle returns Chainlink latestRoundData without proper validation.

Issue

In getPriceFromChainlink function at ConnextPriceOracle.sol#L122, there is no check to see if returned price of aggregator.latestRoundData() is not stale. More details at https://docs.chain.link/docs/faq/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round

Recommended Mitigation Steps

Modify the function as below:

(uint80 round, int256 latestPrice, , uint256 latestTimestamp, uint80 answeredInRound) = aggregator.latestRoundData();
require(feedPrice > 0, "Chainlink price <= 0");
require(answeredInRound >= round, "Stale price");
require(latestTimestamp != 0, "Round not complete");

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/connext/libraries/AssetLogic.sol#L226

Function swapFromLocalAssetIfNeededForExactOut: If _amount is 0 then no need to perform any other step in swapFromLocalAssetIfNeededForExactOut function

if (_amount == 0) {
      return (true, _amount, _asset);
    }

https://github.com/code-423n4/2022-06-connext/blob/main/contracts/contracts/core/shared/ProposedOwnable.sol#L161

_setOwner: This function can be defined as private instead of internal