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: 4/10
Findings: 6
Award: $3,334.78
🌟 Selected for report: 4
🚀 Solo Findings: 0
1103.2637 USDC - $1,103.26
shw
The consumed gas to remove a user's active block is proportional to the total number of array elements (i.e., block numbers). However, the array size can be arbitrarily increased by an attacker (with only paying gas fees), causing a gas griefing attack when the victim wants to fulfill or cancel his transactions.
Consider an attacker performing the following steps:
true for any transferFrom call.prepare function multiple times on the sending chain with invariantData.user being the victim and invariantData.sendingAssetId as the malicious ERC20 token address. The router's address differs in each call to make the digest different every time (to pass the check at line 192). The rest of the provided parameters are valid.activeTransactionBlocks array increases by the number of prepare calls.fulfill or cancel on a regular transaction, he suffers from large gas consumption due to the array traverse and copying in the removeUserActiveBlocks function. Moreover, to cancel those malicious transactions, the user has to wait for them to expire and pay additional gas fees to cancel them.Referenced code: TransactionManager.sol#L209
A possible mitigation is to re-design the way to store the user's active transaction blocks to avoid spending O(N) amount of gas when removing a specific block number from the array (where N is the total elements in the array). Please refer to the issue with the title "Gas optimization on maintaining the activeTransactionBlocks array" for more details.
#0 - LayneHaber
2021-07-12T20:15:02Z
#27
#1 - ghoul-sol
2021-08-02T01:03:29Z
Duplicate of #27 so high risk
402.1396 USDC - $402.14
shw
When the user calls fulfill with a non-zero callTo parameter, the TransactionManager tries to call execute on callTo, and if the function call fails, the manager transfers toSend amount of receiving assets to receivingAddress. However, since the assets may have been transferred before (when the addFunds call fails), the user could get twice the toSend amount assets as a result.
Referenced code: TransactionManager.sol#L405-L408 TransactionManager.sol#L424-L427
Since the assets are approved or transferred to the receivingAddress before calling the execute function, the manager should not transfer assets again if the execute call fails.
#0 - LayneHaber
2021-07-12T19:47:45Z
#46
744.703 USDC - $744.70
shw
When the user calls fulfill on the receiving chain with a non-zero callTo address, the TransactionManager approves callTo to transfer toSend amount of receiving assets. Then, the manager tries to call addFunds on callTo, and if the call fails, the manager then transfers toSend amount of assets to the receivingAddress. However, after the transfer, the callTo address can still transfer the toSend amount of assets from the manager because of the previous approval, resulting in the user getting twice the toSend amount of assets from the manager.
Referenced code: TransactionManager.sol#L387-L389 TransactionManager.sol#L405-L408
Directly transfer toSend amount of assets to callTo before calling addFunds instead of approving callTo.
#0 - LayneHaber
2021-07-12T19:38:09Z
#31
shw
If a user's two transactions can have the same transactionId (while their router addresses differ), then the fulfill and cancel signatures for one of the transaction is replayable on the other.
The only transaction-related data in the SignedCancelData and SignedFulfillData structures are the transactionId of the transaction. Thus, these signatures are valid for all transactions with the same transactionId.
A malicious attacker can monitor the blockchain network and collect all signatures of a victim. Once the attacker found a new victim's transaction created with a duplicated transactionId he has seen before, he can fulfill or cancel the transaction on behalf of the victim since he has a valid signature. Besides, since the signature does not contain information about the chain ID, it is possible to perform the replay attack across chains.
Referenced code: ITransactionManager.sol#L51-L62
Add the router address and the chain ID information to the signed data structure to ensure a signature is only for a specific transaction.
#0 - LayneHaber
2021-07-12T20:21:05Z
#54
74.4703 USDC - $74.47
shw
When a router adds liquidity to the TransactionManager, the manager does not correctly handle the received amount if the transferred token is a deflationary or fee-on-transfer token. The actual received amount is less than that is recorded in the routerBalances variable.
Referenced code: TransactionManager.sol#L97 TransactionManager.sol#L101
Get the received token amount by calculating the difference of token balance before and after the transfer, for example:
uint256 balanceBefore = getOwnBalance(assetId); require(LibERC20.transferFrom(assetId, router, address(this), amount, "addLiquidity: ERC20_TRANSFER_FAILED"); uint256 receivedAmount = getOwnBalance(assetId) - balanceBefore; // Update the router balances routerBalances[router][assetId] += receivedAmount;
#0 - ghoul-sol
2021-08-01T23:43:33Z
While correct, this is a low risk. Number of DeFi protocols are incompatible with "exotic" tokens and it's a user responsibility to know this. Perfect example is rebase tokens and UniswapV2.
🌟 Selected for report: shw
408.6162 USDC - $408.62
shw
The chainId information included in the TransactionManager is immutable, i.e., it could not change after the contract is deployed. However, if a hard fork happens in the future, the contract would become invalid on one of the forked chains because the chain ID has changed.
Referenced code: TransactionManager.sol#L73 TransactionManager.sol#L79
Add a function that allows the admin to set the chainId variable if a hard fork happens.
#0 - LayneHaber
2021-07-12T19:57:13Z
This is a potential issue in the case of a hard fork, but we will not address it for the following reasons:
chainId in the event of a forkchainId gives admins a huge amount of power over the system itselfchainId that could result in unpredictable transaction behaviorInstead, the course of action is to redeploy the contracts with the correct chainId.
101.2401 USDC - $101.24
shw
In general, if a state variable is read more than once, caching its value to a local variable and reusing it will save gas since a storage read spends more gas than a memory write plus a memory read.
Referenced code: TransactionManager.sol#L122-L125 TransactionManager.sol#L254-L260
Rewrite #L122-L125 as follows:
uint256 balance = routerBalances[msg.sender][assetId]; require(balance >= amount, "removeLiquidity: INSUFFICIENT_FUNDS"); // Update router balances routerBalances[msg.sender][assetId] = balance - amount;
Rewrite #L254-L260 as follows:
uint256 balance = routerBalances[invariantData.router][invariantData.receivingAssetId]; require( balance >= amount, "prepare: INSUFFICIENT_LIQUIDITY" ); // Decrement the router liquidity routerBalances[invariantData.router][invariantData.receivingAssetId] = balance - amount;
#0 - sanchaymittal
2021-07-14T12:44:32Z
29.5216 USDC - $29.52
shw
Using the unchecked keyword to avoid redundant arithmetic underflow/overflow checks to save gas when an underflow/overflow cannot happen.
We can apply the unchecked keyword in the following lines of code since there are require statements before to ensure the arithmetic operations would not cause an integer underflow or overflow.
Referenced code: TransactionManager.sol#L125 TransactionManager.sol#L260 TransactionManager.sol#L364 TransactionManager.sol#L520
For example, change the code at line 364 to:
unchecked { uint256 toSend = txData.amount - relayerFee; }
#0 - sanchaymittal
2021-07-14T12:09:54Z
🌟 Selected for report: hrkrshnn
Also found by: GalloDaSballo, cmichel, gpersoon, shw
29.5216 USDC - $29.52
shw
The data structure for storing a user's active transaction blocks can be improved to reduce the time complexity of removing an element from the array, which helps to save gas and avoid unbounded loops. Currently, the consumed gas is proportional to the array size, and thus users could spend a large amount of gas when calling removeUserActiveBlocks if his array size is relatively large.
Referenced code: TransactionManager.sol#L66 TransactionManager.sol#L209 TransactionManager.sol#L555-L557 TransactionManager.sol#L567-L584
Please refer to this link for the implementation.
The activeTransactionBlocks only stores the unique block numbers with active transfers, while the count of active transfers in each block is stored in the activeTransactionBlocksCount array at the corresponding index. The mapping activeTransactionBlocksToIndex records a mapping of the block number to the array index, making removing an element from the array efficient (with constant gas required). Notice that the array returned from getActiveTransactionBlocks is not guaranteed to be sorted, while it is easy for front-end scripts to sort them if needed.
A quick comparison shows that the above implementation is more gas-efficient than the original if the user's activeTransactionBlocks array is more than 17 elements (compiled with 0.8.4 and 200 optimization runs).
#0 - LayneHaber
2021-07-13T17:24:47Z
#60
shw
In the fulfill function, the expiry time must be strictly greater than the current block timestamp to ensure that the expiry has not elapsed (line 318). However, in the cancel function, the current block time can be the same as the expiry time (lines 495 and 533).
Referenced code: TransactionManager.sol#L318 TransactionManager.sol#L495 TransactionManager.sol#L533
Change the comparison at line 318 to >= for consistency.
#0 - LayneHaber
2021-07-12T20:29:12Z
#28