Connext contest - pauliax's results

Handle

pauliax

Vulnerability details

Impact

In addLiquidity function, a router is passed as a sender in LibERC20.transferFrom, not msg.sender, so it basically transfers assets from the router to the contract.

Recommended Mitigation Steps

require(LibERC20.transferFrom(assetId, msg.sender, address(this), amount), "addLiquidity: ERC20_TRANSFER_FAILED");

Handle

pauliax

Vulnerability details

Impact

There is a potential issue in function removeUserActiveBlocks and the for loop inside it. I assume you are aware of block gas limits (they may be less relevant on other chains but still needs to be accounted for), so as there is no limit for activeTransactionBlocks it may grow so large that the for loop may never finish. You should consider introducing an upper limit for activeTransactionBlocks. Also, a malicious actor may block any account (DDOS) by just calling prepare again and again with 0 amount acting as a router. This will push activeTransactionBlocks to the specified user until it is no longer possible to remove them from the array.

This is also a gas issue as function removeUserActiveBlocks iterating and assigning large dynamic arrays is very gas-consuming. Consider optimizing the algorithm, e.g. finding the first occurrence, then swap it with the last item, pop the array, and break. Or maybe even using an EnumerableMap so you can find and remove elements in O(1) https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/structs/EnumerableMap.sol It depends on what is the usual number of activeTransactionBlocks. If it is expected to be low (e.g. less than 5), then the current approach will work, but with larger arrays, I expect EnumerableMap would be more efficient.

Recommended Mitigation Steps

An upper limit will not fully mitigate this issue as a malicious actor can still DDOS the user by pushing useless txs until this limit is reached and a valid router may not be able to submit new txs. As you need to improve both the security and performance of removeUserActiveBlocks I think that EnumerableMap may be a go-to solution.

Handle

pauliax

Vulnerability details

Impact

Both calls to IFulfillHelper (addFunds and execute) are wrapped in separate try/catch statements so basically if addFunds succeeds but execute fails or both of these functions fail, the catch will still send assets to the receivingAddress. I think these should be wrapped to a single try/catch to avoid the potential double spend. Also, I think it is pointless having 2 separate calls (addFunds and execute) as both of them are done one immediately after the other and basically pass the same parameters. One call to the IFulfillHelper could be enough. Eliminating this extra external call will reduce gas costs and simplify the code.

Recommended Mitigation Steps

Make execute payable, send the value, and get rid of addFunds.

Handle

pauliax

Vulnerability details

Impact

Function fulfill first approves the callTo to transfer an amount of toSend tokens and tries to call IFulfillHelper but if the call fails it transfers these assets directly. However, in such case the approval is not reset so a malicous callTo can pull these tokens later: // First, approve the funds to the helper if needed if (!LibAsset.isEther(txData.receivingAssetId) && toSend > 0) { require(LibERC20.approve(txData.receivingAssetId, txData.callTo, toSend), "fulfill: APPROVAL_FAILED"); }

    // Next, call `addFunds` on the helper. Helpers should internally
    // track funds to make sure no one user is able to take all funds
    // for tx
    if (toSend > 0) {
      try
        IFulfillHelper(txData.callTo).addFunds{ value: LibAsset.isEther(txData.receivingAssetId) ? toSend : 0}(
          txData.user,
          txData.transactionId,
          txData.receivingAssetId,
          toSend
        )
      {} catch {
        // Regardless of error within the callData execution, send funds
        // to the predetermined fallback address
        require(
          LibAsset.transferAsset(txData.receivingAssetId, payable(txData.receivingAddress), toSend),
          "fulfill: TRANSFER_FAILED"
        );
      }
    }

Recommended Mitigation Steps

Approve should be placed inside the try/catch block or approval needs to be reset if the call fails.