code-423n4 / 2022-05-cudos-findings

1 stars 0 forks source link

Attackers can prevent the transfer of the highest-value Cosmos to Ethereum transactions #142

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-cudos/blob/de39cf3cd1f1e1cf211819b06d4acf6a043acda0/solidity/contracts/Gravity.sol#L451-L456

Vulnerability details

In order to ensure that profitable batches are eventually created we must avoid locking up the high fee 'good transactions' into obviously bad batches. To add to the difficulty we don't actually know what any token in this process is worth or what ETH gas costs.

The solution this patch provides is to ensure that a new batch always has higher fees than the last batch. This means that even if there are infinite spam transactions, and infinite spamming of the permission less create batch request batch creation will halt long enough for enough good transactions to build up in the pool and a new more profitable batch to be created.

https://github.com/code-423n4/2022-05-cudos/blob/de39cf3cd1f1e1cf211819b06d4acf6a043acda0/spec/batch-creation-spec.md?plain=1#L25-L55

Impact

Attackers can prevent transactions from going through for any gravity bridge

Proof of Concept

The spam solution quoted above assumes that attackers only want to spam by using many low-value transaction. A motivated attacker can instead use a few large-value transactions to always get their malicious transactions into the most high-impact, high-value batches, and cause those batches to always revert.

The submitBatch() function, which is the only method of doing Cosmos to Ethereum bridges, will revert if any one of the internal transfers reverts:

File: solidity/contracts/Gravity.sol   #1

451               // Send transaction amounts to destinations
452               uint256 totalFee;
453               for (uint256 i = 0; i < _amounts.length; i++) {
454                  IERC20(_tokenContract).safeTransfer(_destinations[i], _amounts[i]);
455                  totalFee = totalFee.add(_fees[i]);
456               }

https://github.com/code-423n4/2022-05-cudos/blob/de39cf3cd1f1e1cf211819b06d4acf6a043acda0/solidity/contracts/Gravity.sol#L451-L456

If an attacker submits a large-value transaction for an asset that has transfer hooks (e.g. ERC777, ERC1155, ERC721, etc) and sets up a transfer hook that always reverts, his/her transaction will always be included with other high-value transactions in batches, causing those batches to revert and have to wait for the batch timeout, after which the attacker's transaction will be included with another batch where it will cause failures again. The attacker can submit a few of these transactions to essentially halt the activity of the bridge.

This one is high risk because there is no protection against the attacker using multiple accounts to make things stop indefinitely.

Tools Used

Code inspection

Recommended Mitigation Steps

Have a mechanism of counting the number of times a transaction has been added to a batch, and after some threshold, require that the transfer be done via a new pull mechanism by the recipient

V-Staykov commented 2 years ago

Duplicate of #143