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: 5/10
Findings: 4
Award: $1,943.54
🌟 Selected for report: 1
🚀 Solo Findings: 0
gpersoon
The function removeUserActiveBlocks contains a "for" loop, which depends on the size of the array activeTransactionBlocks. If the array is too large then the for loop will take so much gas that the transaction will revert. The function fulfill, which calls removeUserActiveBlocks will also be reverted and thus the funds cannot be retrieved.
It is possibly to make the array arbitrarily long, by repeatedly calling "prepare" on the destination chain, with the same value for invariantData.user. This wouldn't have to cost a lot, because you can just try to send some random/cheap erc20 token. It only requires gas.
An attacker could choose a destination "user" that frequently receives funds. Or it could front run the transaction and quickly repeatedly call "prepare".
// https://github.com/code-423n4/2021-07-connext/blob/main/contracts/TransactionManager.sol#L164 function prepare( ... activeTransactionBlocks[invariantData.user].push(block.number);
// https://github.com/code-423n4/2021-07-connext/blob/main/contracts/TransactionManager.sol#L567 function removeUserActiveBlocks(address user, uint256 preparedBlock) internal { .. uint256 newLength = activeTransactionBlocks[user].length - 1; ... for (uint256 i; i < newLength + 1; i++) { ... }
// https://github.com/code-423n4/2021-07-connext/blob/main/contracts/TransactionManager.sol#L303 function fulfill( ... removeUserActiveBlocks(txData.user, txData.preparedBlockNumber);
Use a different datastructure to store the blocknumbers, which can be accessed in O(1) instead of O(n). For example a mapping in combination with a linked list.
#0 - LayneHaber
2021-07-12T21:03:14Z
#27
#1 - ghoul-sol
2021-08-02T01:03:26Z
Duplicate of #27 so high risk
402.1396 USDC - $402.14
gpersoon
When the function fulfill tries to call the functions of a receiving contract (callTo) and toSend >0: it first calls addFunds and if that fails that it transfers the funds via transferAsset it secondly calls execute and if that fails that it transfers the funds via transferAsset
Suppose both addFunds and execute fail (because the receivingAddress isn't a contract or it doesn't have these functions or it reverts on these functions), then transferAsset is called twice, thus transferring the funds twice to the receivingAddress. You could transfer funds in this way repeatedly to drain all the funds of the router.
// https://github.com/code-423n4/2021-07-connext/blob/main/contracts/TransactionManager.sol#L394 function fulfill( if (toSend > 0) { try IFulfillHelper(txData.callTo).addFunds{ ... {} catch { require(LibAsset.transferAsset(txData.receivingAssetId, payable(txData.receivingAddress), toSend),"fulfill: TRANSFER_FAILED"); } } try IFulfillHelper(txData.callTo).execute(... {} catch { require(LibAsset.transferAsset(txData.receivingAssetId, payable(txData.receivingAddress), toSend),"fulfill: TRANSFER_FAILED"); } }
Note: a comment shows: Helpers should internally track funds to make sure no one user is able to take all funds for tx This seems to indicate that the helpers should also be trusted, as they are not part of the audit this is difficult to determine. And also it not necessary and logical to trust the helpers to fix this issue.
set toSend to 0 after transferAsset Perhaps call the "execute" function with the original version of toSend
#0 - LayneHaber
2021-07-12T19:45:01Z
#46
🌟 Selected for report: gpersoon
408.6162 USDC - $408.62
gpersoon
The function wrapCall is not completely safe for all possible ERC20 contracts.
If the returnData.length is larger than 1 the "abi.decode(returnData, (bool));" will fail. Which means the interactions with that ERC20 contract will fail. Although this is unlikely, it is easy to protect against it.
// https://github.com/code-423n4/2021-07-connext/blob/main/contracts/lib/LibERC20.sol#L21 function wrapCall(address assetId, bytes memory callData) internal returns (bool) { ... (bool success, bytes memory returnData) = assetId.call(callData); LibUtils.revertIfCallFailed(success, returnData); return returnData.length == 0 || abi.decode(returnData, (bool)); }
Change return returnData.length == 0 || abi.decode(returnData, (bool)); to: return (returnData.length == 0) || (returnData.length == 1 && abi.decode(returnData, (bool)));
#0 - LayneHaber
2021-07-13T21:52:05Z
🌟 Selected for report: hrkrshnn
Also found by: GalloDaSballo, cmichel, gpersoon, shw
29.5216 USDC - $29.52
gpersoon
The function removeUserActiveBlocks is not very gas efficient and can even revert when the array gets too large. (also see issue "Grief a user by not allowing him to retrieve funds" )
See below for a solution with a mapping and a linked list. Note: this will also help with the issue "Grief a user by not allowing him to retrieve funds"
// https://github.com/code-423n4/2021-07-connext/blob/main/contracts/TransactionManager.sol#L567 function removeUserActiveBlocks(address user, uint256 preparedBlock) internal { // Remove active blocks uint256 newLength = activeTransactionBlocks[user].length - 1; uint256[] memory updated = new uint256; bool removed = false; uint256 updatedIdx = 0; for (uint256 i; i < newLength + 1; i++) { // Handle case where there could be more than one tx added in a block // And only one should be removed if (!removed && activeTransactionBlocks[user][i] == preparedBlock) { removed = true; continue; } updated[updatedIdx] = activeTransactionBlocks[user][i]; updatedIdx++; } activeTransactionBlocks[user] = updated; }
struct Record { uint256 Count; uint256 PrevBlocknr; uint256 NextBlocknr; } mapping(address=> mapping(uint256 => Record)) public activeTransactionBlocks;
function addUserActiveBlocks(addressuser, uint256 newBlock) public {
if (activeTransactionBlocks[user][newBlock].Count==0) {
uint PrevBlocknr = activeTransactionBlocks[user][0].PrevBlocknr; // can be 0
activeTransactionBlocks[user][PrevBlocknr].NextBlocknr = newBlock;
activeTransactionBlocks[user][newBlock].PrevBlocknr = PrevBlocknr;
activeTransactionBlocks[user][0].PrevBlocknr=newBlock;
}
activeTransactionBlocks[user][newBlock].Count++;
}
function removeUserActiveBlocks(addressuser, uint256 oldBlock) public { activeTransactionBlocks[user][oldBlock].Count--; if (activeTransactionBlocks[user][oldBlock].Count==0) { uint PrevBlocknr = activeTransactionBlocks[user][oldBlock].PrevBlocknr; uint NextBlocknr = activeTransactionBlocks[user][oldBlock].NextBlocknr; activeTransactionBlocks[user][PrevBlocknr].NextBlocknr=NextBlocknr; activeTransactionBlocks[user][NextBlocknr].PrevBlocknr=PrevBlocknr; activeTransactionBlocks[user][oldBlock].PrevBlocknr=0; activeTransactionBlocks[user][oldBlock].NextBlocknr=0; } }
All active blocks can be retrieved by following the links in activeTransactionBlocks, starting by activeTransactionBlocks[user][0]
#0 - LayneHaber
2021-07-13T17:22:14Z
#60