Platform: Code4rena
Start Date: 09/07/2021
Pot Size: $25,000 USDC
Total HM: 7
Participants: 10
Period: 3 days
Judge: ghoulsol
Total Solo HM: 2
Id: 19
League: ETH
Rank: 1/10
Findings: 6
Award: $7,790.90
🌟 Selected for report: 12
🚀 Solo Findings: 1
1103.2637 USDC - $1,103.26
0xRajeev
The addLiquidity() function takes a router address parameter whose liquidity is increased (instead of assuming that router == msg.sender like done on removeLiquidity()) on this contract/chain by transferring the fund amount from router address to this contract if assetID != 0 i.e. ERC20 tokens. However, anyone can call this function on the router’s behalf. For assetID == 0, the Ether transfer via msg.value comes from msg.sender and hence is assumed to be the router itself.
Impact: This will allow anyone to call this function and arbitrarily move ERC20 tokens from router address to this contract, assuming router has given max approval to this contract and has assetID amount available for transfer. While the router can always remove the liquidity if it doesn’t want to maintain that level of liquidity, this lack of access control or flexibility for a relayer to add liquidity on router’s behalf may unnecessarily (and without authorization) increase the router’s exposure to protocol risk to more than it desires.
Use of msg.sender in removeLiquidity: https://github.com/code-423n4/2021-07-connext/blob/8e1a7ea396d508ed2ebeba4d1898a748255a48d2/contracts/TransactionManager.sol#L88-L98
Manual Analysis
Consider the use of msg.sender in addLiquidity() or evaluate this risk otherwise.
#0 - LayneHaber
2021-07-13T15:31:29Z
The bigger issue here is the typo here, if we use the funds from msg.sender that means that people are donating funds to the router.
We will fix the msg.sender, but allow addLiquidity to be callable by anyone
#1 - ghoul-sol
2021-08-01T23:38:29Z
This is high risk because funds can be send to the wrong address.
402.1396 USDC - $402.14
0xRajeev
During fulfill() on the receiving chain, if the user has set up an external contract at txData.callTo, the catch blocks for both IFulfillHelper.addFunds() and IFulfillHelper.excute() perform transferAsset to the predetermined fallback address txData.receivingAddress.
If addFunds() has reverted earlier, toSend amount would already have been transferred to the receivingAddress. If execute() also fails, it is again transferred.
Scenario: User sets up receiver chain txData.callTo contract such that both addFunds() and execute() calls revert and that will let him get twice the toSend amount credited to the receivingAddress. So effectively, Alice locks 100 tokenAs on chain A and can get 200 tokenAs (or twice the amount of any token she is supposed to get on chainB from the router), minus relayer fee, on chainB. Router liquidity is double-dipped by Alice and router loses funds.
Manual Analysis
The second catch block for execute() should likely not have the transferAsset() call. It seems like a copy-and-paste bug unless there is some reason that is outside the specified scope and documentation for this contest.
#0 - LayneHaber
2021-07-14T18:01:44Z
🌟 Selected for report: 0xRajeev
4086.1617 USDC - $4,086.16
0xRajeev
The cancelling relayer is being paid in receivingAssetId on the sendingChain instead of in sendingAssetID. If the user relies on a relayer to cancel transactions and that receivingAssetId asset does not exist on the sending chain (assuming only sendingAssetID on the sending chain and receivingAssetId on the receiving chain are assured to be valid and present) then the cancel transaction from the relayer will always revert and user’s funds will remain locked on the sending chain.
Impact: Expired transfers can never be cancelled and user funds will be locked forever if user relies on a relayer.
Manual Analysis
Change receivingAssetId to sendingAssetId in transferAsset() on L514.
#0 - LayneHaber
2021-07-14T01:24:32Z
330.9791 USDC - $330.98
0xRajeev
The signature check in recoverFulfillSignature() only uses transaction ID (along with the relayer fee) which can be accidentally reused by the user, in which case the older signatures with the older relayer fees can be replayed. The signature should be on the entire digest hashInvariantTransactionData(txData) as indicated in the comment on L306.
Impact: If the user signatures are indeed on the digest as indicated by the comment, the signature/address check in fulfill() will fail. If not, they may be accidentally/intentionally replayed with same transaction ID which also appears to be an outstanding question as indicated by the comment on L12.
recoverCancelSignature() similarly uses only tx ID.
Unless there is a good reason not to, it is safer to include hashInvariantTransactionData(txData) in signatures so that they cannot be replayed with different txData (but same tx ID) whose preparedBlockNumber is > 0.
Manual Analysis
Evaluate if the signature should contain only tx ID or the entire digest, and change logic appropriately.
#0 - LayneHaber
2021-07-12T20:23:17Z
User should be able to break up large transfers across multiple routers using the same transactionId to keep the transaction unlocking atomic. For example, say I want to transfer $100K, but there are only 8 routers who each have $60K available. I should be able to break up the single transaction into $20K transactions split across 5 of the routers. When unlocking this, I should only need to broadcast a single signature, so all of the transactions can be unlocked simultaneously.
#1 - ghoul-sol
2021-08-02T00:04:19Z
Bumping to medium risk as replay attack can have significant consequences
183.8773 USDC - $183.88
0xRajeev
The protocol appears to allow arbitrary assets, amounts and routers/users without an initial time-bounded whitelist of assets/routers/users or upper bounds on amounts. Also, there is no pause/unpause functionality. While this lack of ownership and control makes it completely permissionless, it is a risky design because if there are latent protocol vulnerabilities there is no fallback option.
Lack of owner, whitelisting, thresholds, pause/unpause in the protocol.
See https://medium.com/electric-capital/derisking-defi-guarded-launches-2600ce730e0a
Manual Analysis
Consider an initial guarded launch approach to owner-based whitelisting asset types, router/recipient addresses, amount thresholds and adding a pause/unpause functionality for emergency handling. The design should be able to make this owner configurable where the owner can renounce ownership at a later point when the protocol operation is sufficiently time-tested and deemed stable/safe.
#0 - LayneHaber
2021-07-15T01:02:17Z
0xRajeev
Connext allows the use any ERC20 assets to be used for cross-chain transfers. Some of these tokens could charge a fee, add a reward or rebase over time. However, the protocol does not have the required support to handle such tokens. The necessary checks include at least verifying the amount of tokens transferred to/from contracts before and after the actual transfer to infer any fees/interest/reward/rebasing. These seem to be absent in the various functions.
Impact: Users transferring rebasing/deflationary/inflationary tokens accidentally/maliciously will lead to miscalculations of balances in protocol.
See finding https://consensys.net/diligence/audits/2021/03/umbra-smart-contracts/#document-token-behavior-restrictions
Manual Analysis
Add necessary checks including at least verifying the amount of tokens transferred to/from contracts before and after the actual transfer to infer any fees/interest/reward/rebasing. Alternatively whitelist/blacklist allowed/disallowed assetIDs.
#0 - LayneHaber
2021-07-12T20:06:28Z
#68
183.8773 USDC - $183.88
0xRajeev
Zero-address checks are in general a best-practice. However, addLiquidity() and removeLiquidity() are missing zero-address checks on router and recipient addresses respectively.
addLiquidity() on Eth transfers will update the zero index balance and get logged as such in the event without the amount getting accounted for the correct router.
For ERC20 assets, token.transfer() generally implements this check but the Eth transfer using transferEth() does not have this check and calls addr.call(value) which will lead to burning in the case of removeLiquidity().
The checks may be more important because assetID is 0 for Eth. So a router may accidentally use 0 values for both assetID and router/recipient.
There is also a missing zero-address check on sendingChainFallback which is relevant for Eth transfers in cancel(). The comment on L178 indicates the need for this but the following check on L179 ends up checking receivingAddress instead (which is also necessary).
https://github.com/code-423n4/2021-07-connext/blob/8e1a7ea396d508ed2ebeba4d1898a748255a48d2/contracts/TransactionManager.sol#L88 https://github.com/code-423n4/2021-07-connext/blob/8e1a7ea396d508ed2ebeba4d1898a748255a48d2/contracts/TransactionManager.sol#L101-L104
https://github.com/code-423n4/2021-07-connext/blob/8e1a7ea396d508ed2ebeba4d1898a748255a48d2/contracts/TransactionManager.sol#L116 https://github.com/code-423n4/2021-07-connext/blob/8e1a7ea396d508ed2ebeba4d1898a748255a48d2/contracts/TransactionManager.sol#L128-L131
Manual Analysis
Add zero-address checks.
#0 - sanchaymittal
2021-07-14T09:09:49Z
183.8773 USDC - $183.88
0xRajeev
The prepare() function hashes the invariantData parameter data to check the mapping entry is 0 for that digest as a measure to prevent duplicate prepare()s. However, an attacker can abuse this check to front-run a targeted victim's prepare Tx with the same parameters and with some dust amount to prevent the user’s actual prepare Tx from succeeding.
Impact: Potential griefing attack vector if user address is not msg.sender. This is with the assumption that relayers are only relevant on the receiving side where the user may not have the receivingAssetId i.e. no reason for msg.sender of prepare() to be the relayer and not the user.
Manual Analysis
Add msg.sender == invariantData.user check on sending chain side similar to the check for router address on the receiving side.
#0 - LayneHaber
2021-07-15T02:09:40Z
#1 - LayneHaber
2021-07-17T00:09:11Z
Reverted the changes, without tracking the active blocks this DOS vector still exists but presents no risk to locked funds. It is also unclear what the attacker would gain from this, and would cost them gas funds.
Could add a signature, but comes with some UX drawbacks that seem more important than removing the DOS vector
0xRajeev
The expiry check against block.timestamp in fulfill() is stricter using ‘>’ instead of ‘>=‘ for an open-interval instead of closed like elsewhere.
Impact: This will cause fulfill() to expire earlier than it should.
Manual Analysis
Make inequality less strict (use ‘>=‘) for time to be a closed-interval like other checks on L188, L495 and L533.
#0 - LayneHaber
2021-07-12T20:28:47Z
#28
🌟 Selected for report: 0xRajeev
224.978 USDC - $224.98
0xRajeev
EIP-2929 in Berlin fork increased the gas costs of SLOADs and CALL* family opcodes increasing them for not-accessed slots/addresses and decreasing them for accessed slots. EIP-2930 optionally supports specifying an access list (in the transaction) of all slots and addresses accessed by the transaction which reduces their gas cost upon access and prevents EIP-2929 gas cost increases from breaking contracts.
Impact: Considering these changes may significantly impact gas usage for transactions that call functions touching many state variables or making many external calls. Specifically, removeUserActiveBlocks() removes an active block from the array of blocks for an user, all of which are stored in storage. Transactions for fulfill() and cancel() functions that call removeUserActiveBlocks() can consider using access lists for all the storage state (of user’s active blocks) they touch (read + write) to reduce gas.
https://eips.ethereum.org/EIPS/eip-2929
https://eips.ethereum.org/EIPS/eip-2930
https://hackmd.io/@fvictorio/gas-costs-after-berlin
https://github.com/gakonst/ethers-rs/issues/265
Manual Analysis
Evaluate the feasibility of using access lists to save gas due to EIPs 2929 & 2930 post-Berlin hard fork. The tooling support is WIP.
#0 - LayneHaber
2021-07-15T19:21:03Z
Removed tracking of active blocks: https://github.com/connext/nxtp/pull/24
101.2401 USDC - $101.24
0xRajeev
Post-Berlin, SLOADs on state variables accessed first-time in a transaction increased from 800 gas to 2100, which is a 2.5x increase. Successive SLOADs cost 100 gas. Memory stores/loads (MSTOREs/MLOADs) cost only 3 gas. Therefore, by caching repeatedly read state variables in local variables within a function, one can save >=100 gas.
In removeLiquidity(), caching routerBalances[msg.sender][assetId] in a local variable will save 1002 = 200 gas (mapping of a mapping) because it is SLOADed 2 times, once in the require() check and then again to increment it: https://github.com/code-423n4/2021-07-connext/blob/8e1a7ea396d508ed2ebeba4d1898a748255a48d2/contracts/TransactionManager.sol#L122-L125. Instead, change to: uint256 currentBalance = routerBalances[msg.sender][assetId]; require(currentBalance >= amount, "removeLiquidity: INSUFFICIENT_FUNDS"); // Update router balances routerBalances[msg.sender][assetId] = currentBalance - amount In prepare(), caching routerBalances[msg.sender][assetId] in a local variable will save 1002 = 200 gas (mapping of a mapping) because it is SLOADed 2 times, once in the require() check and then again to increment it: https://github.com/code-423n4/2021-07-connext/blob/8e1a7ea396d508ed2ebeba4d1898a748255a48d2/contracts/TransactionManager.sol#L253-L260 In function removeUserActiveBlocks(), caching activeTransactionBlocks[user][i] in a local variable will save 100*2 = 200 gas (mapping of a mapping) per active block, because it is SLOADed 2 times, once in the conditional predicate check and again in the assignment RHS: https://github.com/code-423n4/2021-07-connext/blob/8e1a7ea396d508ed2ebeba4d1898a748255a48d2/contracts/TransactionManager.sol#L576-L580. Instead change to: uint256 block = activeTransactionBlocks[user][i]; if (!removed && block == preparedBlock) { removed = true; continue; } updated[updatedIdx] = block;
Manual Analysis
Cache repeatedly read state variables (especially those within a loop) in local variables at an appropriate part of the function (preferably the beginning) and use them instead of state variables. Converting repetitive SLOADs to MLOADs reduces gas from 100 to 3.
#0 - LayneHaber
2021-07-13T17:28:35Z
#75
101.2401 USDC - $101.24
0xRajeev
EIP-2929 in Berlin fork increased the gas costs of CALL* family opcodes to 2600. Making a delegatecall to a library function therefore costs 2600. LibUtils.revertIfCallFailed() reverts and passes on the revert string if the boolean argument is false. Instead, moving the checking of the boolean to the caller avoids the library call when the boolean is true, which is likely the case most of the time.
Manual Analysis
Remove the boolean parameter from revertIfCallFailed() and move the conditional check logic to the call sites.
#0 - LayneHaber
2021-07-14T15:52:34Z
🌟 Selected for report: 0xRajeev
224.978 USDC - $224.98
0xRajeev
While code modularity is generally a good practice and creating libraries of functions commonly used across different contracts can increase maintainability and reduce contract deployment size/cost, it comes at the increased cost of gas usage at runtime because of the external calls. EIP-2929 in Berlin fork increased the gas costs of CALL* family opcodes to 2600. Making a delegatecall to a library function therefore costs 2600.
Impact: A LibAsset.transferAsset() call from TransactionManager.sol makes LibERC20.transfer() call for ERC20 which in turn makes another external call to LibUtils.revertIfCallFailed() in wrapCall. So an ERC20 transfer effectively makes 3 additional (besides the ERC20 token contract function call assetId.call(..) external calls -> LibAsset -> LibERC20 -> LibUtils, which costs 2600*3 = 7800 gas.
Combining these functions into a single library or making them all internal to TransactionManager.sol can convert these delegatecalls into JMPs to save gas.
And other Lib* calls.
Manual Analysis
Consider moving all the library functions internal to this contract or to a single library to save gas from external calls each of which costs 2600 gas.
#0 - LayneHaber
2021-07-13T21:52:15Z
🌟 Selected for report: 0xRajeev
224.978 USDC - $224.98
0xRajeev
While it may be considered extra-safe to have a nonreentrant modifier on all functions making any external calls even though they are to trusted contracts, when functions implement Checks-Effects-Interactions (CEI) pattern, it is helpful to evaluate the perceived security benefit vs gas usage trade-off for using nonreentrant modifier.
Functions adhering to the CEI pattern may consider not having the nonreentrant modifier which does two SSTORES (getting more expensive with the London fork EIP-3529) to its _status state variable.
Example 1: In addLiquidity(), by moving the updating of router balance on L101 to before the transfers from L92, the function would adhere to CEI pattern and could be evaluated to remove the nonreentrant modifier.
Example 2: removeLiquidity() already adheres to CEI pattern and could be evaluated to remove the nonreentrant modifier.
prepare() can be slightly restructured to follow CEI pattern as well. However, fulfill() and cancel() are risky with multiple external calls and its safer to leave the nonreentrant call at the expense of additional gas costs.
Impact: Save gas by removing nonreentrant modifier if function is deemed to be reentrant safe. This can save gas costs of 2 SSTORES per function call that uses this modifier: _status SSTORE from 1 to 2 costs 5000 and _status SSTORE from 2 to 1 which costs 100 (because it was already accessed) which is significant at 5100 per call post-Berlin EIP-2929.
Manual Analysis
Evaluate security benefit vs gas usage trade-off for using nonreentrant modifier on functions that may already be reentrant safe or do not need this protection. It may indeed be safe to leave this modifier (while accepting the gas impact) if such an evaluation is tricky or depends on assumptions.
#0 - LayneHaber
2021-07-15T19:17:59Z
29.5216 USDC - $29.52
0xRajeev
The router balance checks are performed within a require and then Solidity 0.8.4’s built-in arithmetic overflow/underflow checks again perform this check implicitly (and revert on underflows) during the subtraction of the balance. So effectively, this router balance >= amount check is performed twice (and both times by reading state variables via SLOADs). This redundant check costs a bit of gas.
Manual Analysis
Remove the initial require() check to let Solidity perform the underflow check in the subtraction but this comes at the cost of having only a revert and missing the error string from require(). Alternatively, put the router balance decrement expression within unchecked{} block to save gas by avoiding Solidity 0.8.4’s built-in arithmetic overflow/underflow checks because this has already been explicitly performed within require().
#0 - LayneHaber
2021-07-13T17:33:00Z
#74
🌟 Selected for report: 0xRajeev
224.978 USDC - $224.98
0xRajeev
Checking if toSend > 0 before making the external library call to LibAsset.transferAsset() can save 2600 gas by avoiding the external call in such situations.
Manual Analysis
Add toSend > 0 to predicate on L375 similar to check on L387.
#0 - sanchaymittal
2021-07-14T08:46:51Z