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

Potential NFT Lock-up Due to Lack of Contract Account Verification in L1 depositTokens() Function

kildren

Relevant GitHub Links

transferFrom operation in depositTokens()

safeTransferFrom execution in _cancelRequest()

Summary

The depositTokens() function on the L1 side does not perform checks to verify if the sender is a contract account. This oversight can lead to complications during the cancelRequest() process, potentially resulting in NFTs being locked within the contract.

Vulnerability Details

  1. In depositTokens():

    • The function does not perform any checks to verify if the sender (msg.sender) is a contract account.

    • It does not require the implementation of onERC721Received() for contract accounts.

  2. In cancelRequest():

    • The function uses safeTransferFrom() to return NFTs.

    • safeTransferFrom() requires contract recipients to implement onERC721Received().

This inconsistency can lead to a situation where NFTs are accepted from contract accounts during deposit but cannot be returned to them during cancellation.

Impact

NFT Lock-up: If a contract account that doesn't implement onERC721Received() deposits an NFT and then attempts to cancel the request, the NFT will become locked in the escrow contract.

POC

Create a file JustContract.t.sol in the test/mock folder:

// SPDX-License-Identifier: UNLICENSED\
pragma solidity ^0.8.0;
​
contract JustContract {
​
}

And add the following to test/Bridge.t.sol:

import "./mock/JustContract.t.sol";
​
function test_cancelRequest_forContract() public {
uint256[] memory ids = new uint256[](2);
ids[0] = 0;
ids[1] = 9;
​
JustContract c = new JustContract();
vm.deal(address(c), 10 ether);
​
(uint256 nonce, uint256[] memory payload) = setupCancelRequest(address(c), ids);
assert(IERC721(erc721C1).ownerOf(ids[0]) != address(c));
assert(IERC721(erc721C1).ownerOf(ids[0]) != address(c));
​
Request memory req = Protocol.requestDeserialize(payload, 0);
​
vm.expectEmit(true, false, false, false, bridge);
emit CancelRequestStarted(req.hash, 42);
vm.breakpoint("b");
IStarklane(bridge).startRequestCancellation(payload, nonce);
​
vm.expectEmit(true, false, false, false, bridge);
emit CancelRequestCompleted(req.hash, 42);
IStarklane(bridge).cancelRequest(payload, nonce);
​
assert(IERC721(erc721C1).ownerOf(ids[0]) == address(c));
assert(IERC721(erc721C1).ownerOf(ids[1]) == address(c));
}

Run the test: forge test --mt test\_cancelRequest\_forContract -vvvv

You will get the following output:

.......
│ │ ├─ \[5707] ERC721MintFree::safeTransferFrom(ERC1967Proxy: \[0xc7183455a4C133Ae270771860664b6B7ec320bB1], JustContract: \[0xa0Cb889707d426A7A386870A03bc70d1b0697598], 0)\
│ │ │ ├─ emit Transfer(from: ERC1967Proxy: \[0xc7183455a4C133Ae270771860664b6B7ec320bB1], to: JustContract: \[0xa0Cb889707d426A7A386870A03bc70d1b0697598], tokenId: 0)\
│ │ │ ├─ \[24] JustContract::onERC721Received(ERC1967Proxy: \[0xc7183455a4C133Ae270771860664b6B7ec320bB1], ERC1967Proxy: \[0xc7183455a4C133Ae270771860664b6B7ec320bB1], 0, 0x)\
│ │ │ │ └─ ← \[Revert] EvmError: Revert\
│ │ │ └─ ← \[Revert] revert: ERC721: transfer to non ERC721Receiver implementer\
│ │ └─ ← \[Revert] revert: ERC721: transfer to non ERC721Receiver implementer\
│ └─ ← \[Revert] revert: ERC721: transfer to non ERC721Receiver implementer\

Evidently, the contract fails to retrieve the NFT after depositing it to the bridge.

Tools Used

Manual Review

Recommendations

Implement Consistent Checks: Add a check in depositTokens() to verify if the sender is a contract account. If so, require it to implement onERC721Received():

function depositTokens(
uint256 salt,
address collectionL1,
snaddress ownerL2,
uint256[] calldata ids,
bool useAutoBurn
)
external
payable
{
+ address sender = msg.sender;
+ if (sender.code.length > 0) {
+ require(
+ ERC165(sender).supportsInterface(ERC721TokenReceiver.onERC721Received.selector),
+ "Sender must implement onERC721Received"
+ );
......
}
Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational / Gas

Please, do not suppose impacts, think about the real impact of the bug and check the CodeHawks documentation to confirm: https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity A PoC always helps to understand the real impact possible.

Support

FAQs

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

Give us feedback!