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: 2/10
Findings: 7
Award: $5,166.54
🌟 Selected for report: 4
🚀 Solo Findings: 1
cmichel
The addLiquidity function can be called by anyone to transfer funds from the router address specified as a function argument.
These funds must be approved first by the router prior to calling this function. There are different griefing attacks depending on the allowance that the router set to the contract:
allowance > amount: For example, the allowance could be set to the maximum value of type(uint256).max which allows anyone to deposit more of the router's balance to the contract, even if they only intended to deposit amount. The additional funds must then first be removed again by the router. Usually, it's best practice to only set an allowance equal to the amount to transfer. (option 2)allowance = amount: Upon observing an addLiquidity(amount, assetId, router) transaction, the attacker can call addLiquidity(1, assetId, router) to transfer and decrease the allowance by a single wei. As the updated allowance for the original call with then be allowance - 1 < amount, the original addLiquidity call will revert.A griefer can deny anyone from depositing their desired amount of liquidity.
Remove the router argument and always transfer from msg.sender, essentially setting router = msg.sender.
#0 - LayneHaber
2021-07-12T20:01:05Z
#48
402.1396 USDC - $402.14
cmichel
The fulfill transaction on the receiving chain tries to call the addFunds and execute actions on txData.callTo.
When any of the calls reverts, the funds are sent to the txData.receivingAddress.
The txData.callTo is user-controlled and an attacker can deploy a contract to this address and revert on both calls, receiving the amount twice.
The receiver can get twice the amounts of funds that the router agreed to (and locked up). This allows stealing all added liquidity in the contract by an attacker engaging in cross-chain transfers with themselves.
Only transfer them once, not in both catch blocks.
#0 - LayneHaber
2021-07-12T19:44:15Z
#46
744.703 USDC - $744.70
cmichel
The fulfill transaction on the receiving chain first approves the txData.callTo contract with the toSend amount.
It then tries to call the addFunds and execute actions on txData.callTo.
When any of the calls reverts, the funds are sent to the txData.receivingAddress.
Note this is a different attack from "Funds are sent twice on callTo errors". Assume that issue was already fixed and it would only send out the transfer once.
The txData.callTo is user-controlled and an attacker can deploy a contract to this address and revert on all calls.
They then receive the funds once (or twice if other issue was not fixed) through the direct LibAsset.transferAsset(txData.receivingAssetId, payable(txData.receivingAddress), toSend) call.
Afterwards, as the approval was given to callTo, the attacker can send a transaction to their callTo contract to transferFrom(TransactionManager, toSend) again.
The receiver can get twice the amounts of funds that the router agreed to (and locked up). This allows stealing all added liquidity in the contract by an attacker engaging in cross-chain transfers with themselves.
Giving approvals to arbitrary contracts is very dangerous. It should probably be removed completely.
If it's really desired, at least reset the approval to 0 again at the end of the fulfill function such that callTo must have used transferFrom within the same transaction in the execute call.
#0 - LayneHaber
2021-07-12T19:37:15Z
#31
🌟 Selected for report: cmichel
1225.8485 USDC - $1,225.85
cmichel
The agreement between the user and the router seems to already happen off-chain because all the fields are required for the initial InvariantTransactionData call already.
A router could pretend to take on a user's cross-chain transfer, the user sends their prepare transaction locking up funds on the sending chain.
But then the router simply doesn't respond or responds with a prepare transaction of amount=0.
The user's funds are then locked for the entire expiry time whereas the router does not have to lock up anything as the amount is 0, even no gas if they simply don't respond. This way a router can bid on everything off-chain without a penalty and take down everyone that accepts the bid.
Maybe there must be a penalty mechanism for non-responsive routers that agreed off-chain, slashing part of their added liquidity. Could also be that the bid signature already helps with this, but I'm not sure how it works as the off-chain part is not part of the repo.
#0 - LayneHaber
2021-07-12T20:58:15Z
This is true, and we are building penalty mechanisms outside of these contracts. For now we are considering adding in a permissioned launch, see #49
cmichel
The fulfill signature is only on (txData.transactionId, relayerFee) which allows the router to steal user funds for cross-chain transfers that go to the same router and use the same transaction ID as an earlier transfer.
Example:
fulfill.signature which acts as a ticket for the router to claim their amount on the sending chain.prepare at all now, they can just use the previous signature to steal the fundsNote that the signature is also not bound to a chain, the user could create the second cross-chain transfer on a different chain using different assets.
The transaction ID must be unique per user among all chains. Therefore, it's hard to enforce this on a chain-specific contract.
A way to mitigate this would be to let the signature be on the entire txData + relayerFee instead of just the txData.transactionId + relayerFee.
#0 - LayneHaber
2021-07-12T20:23:29Z
#54
#1 - ghoul-sol
2021-08-02T00:05:22Z
Medium risk Ref #54
cmichel
There are ERC20 tokens that made certain customizations to their ERC20 contracts.
One type of these tokens is deflationary tokens that charge a certain fee for every transfer() or transferFrom().
Others are rebasing tokens that increase in value over time like Aave's aTokens (balanceOf changes over time).
The prepare/addLiquidity functions will credit the full amount for tokens even though they might have received less due to a fee.
This allows the router/user to claim the pre-fee amount later in fulfill stealing from another user.
In the case of rebalancing tokens, the initial locked amount in prepare might have already increased in value and the increase stays in the contract and cannot be withdrawn.
Users should be made aware of these restrictions. One possible mitigation is to measure the asset change right before and after the asset-transferring routines.
#0 - LayneHaber
2021-07-12T20:07:19Z
#68
#1 - ghoul-sol
2021-08-01T23:45:14Z
Ref #68 low risk
cmichel
Upon observing a prepare transaction, an attacker can frontrun it with the same invariantData but an amount of a single wei.
This inserts a value into variantTransactionData[digest] and the original transcation will fail because of the require(variantTransactionData[digest] == bytes32(0), "prepare: DIGEST_EXISTS") check.
Note that this works because invariantData.user is never required to be the msg.sender.
The cost of the attack is gas + a single wei of the sending assets.
A griefer can deny anyone from creating cross-chain transfers at near-zero cost.
A quick fix would be to require msg.sender == invariantData.user if invariantData.sendingChainId == chainId.
#0 - LayneHaber
2021-07-12T21:01:45Z
#52
#1 - ghoul-sol
2021-08-01T23:56:06Z
This is low-risk because it's not clear what the attacker would gain and the cost for the attacker is gas.
🌟 Selected for report: cmichel
408.6162 USDC - $408.62
cmichel
Some ERC20 tokens like USDT require resetting the approval to 0 first before being able to reset it to another value. (See Line 201)
The LIibERC20.approve function does not do this - unlike OpenZeppelin's safeApprove implementation.
Repeated USDT cross-chain transfers to the same user on receiving chain = ETH mainnet can fail due to this line not resetting the approval to zero first:
require(LibERC20.approve(txData.receivingAssetId, txData.callTo, toSend), "fulfill: APPROVAL_FAILED");
LiibERC20.approve should do two approve calls, one setting it to 0 first, then the real one.
Check OpenZeppelin's safeApprove.
#0 - LayneHaber
2021-07-13T21:52:22Z
🌟 Selected for report: cmichel
408.6162 USDC - $408.62
cmichel
The user's fulfill signature on the receiving chain is at the same time used by the router as a way to claim their amount on the sending chain.
If the sending chain's expiry date has passed, the user can cancel this side of the transfer and claim back their deposit before the router can claim it.
Therefore, the comment that the receiving chain's expiry needs to be decreased is correct:
// expiry should be decremented to ensure the router has time to complete the sender-side transaction after the user completes the receiver-side transactoin.
However, this is not enforced.
If a wrong expiry date is chosen by the router, or the sender congests the network long enough such that the router's fulfill transaction does not get through, the router loses their claim and the user gets a free cross-chain transfer.
It would be possible to enforce that receivingSide.expiry + buffer < sendingSide.expiry if the original expiry was part of the invariant data.
This would programmatically avoid errors like the ones mentioned. (Assuming all supported chains use the same way to measure time / use UNIX timestamps.)
#0 - LayneHaber
2021-07-12T20:56:00Z
This is true, but the router is definitely incentivized to do this correctly. Adding this would also require adding an additional {MINIMUM/MAXIMUM}_BUFFER, and increases the complexity of the contracts for relatively minimal benefit
29.5216 USDC - $29.52
cmichel
The compiler version is set to 0.8.4 which already has overflow checks. One could remove the unnecessary ones to save on gas:
// removeLiquidity require(routerBalances[msg.sender][assetId] >= amount, "removeLiquidity: INSUFFICIENT_FUNDS"); // prepare require( routerBalances[invariantData.router][invariantData.receivingAssetId] >= amount, "prepare: INSUFFICIENT_LIQUIDITY" );
#0 - LayneHaber
2021-07-13T17:34:26Z
#74
🌟 Selected for report: hrkrshnn
Also found by: GalloDaSballo, cmichel, gpersoon, shw
29.5216 USDC - $29.52
cmichel
The removeUserActiveBlocks function removes a single element from an array by iterating over the entire array and copying each element except the element to delete.
This is rather inefficient.
One can iterate over the array once to find the index of the element to remove (and then break out of the loop, saving iterations).
To delete the element at the found index k, one moves the last element to index k and then removes the last (now duplicated) element by popping off.
This works as activeTransactionBlocks is not required to be sorted and the order does not matter.
#0 - LayneHaber
2021-07-13T17:20:23Z
#60
🌟 Selected for report: cmichel
224.978 USDC - $224.98
cmichel
Both the recoverFulfillSignature and recoverCancelSignature functions take a large TransactionData object as their first argument but only use the transactionId field of the struct.
It should be more efficient to only pass txData.transactionId as the parameter.
#0 - LayneHaber
2021-07-13T22:06:59Z