Relevant GitHub Links
req.onwerL1 in depositTokens()
transferFrom operation in depositTokens()
req.onwerL1 will be used again in _cancelRequest()
Summary
The L1 depositTokens() function lacks proper interoperability with smart contracts. Due to ERC721's transferFrom requirement that 'from' must be the owner, users intending to interact with ArkProject through smart contracts are forced to first transfer NFT ownership to their authorized contract. From ArkProject's perspective, the NFT ownership remains with the authorized contract. In the event of a cancelRequest(), ArkProject will transfer the NFT to the interacting contract rather than the original NFT owner, potentially resulting in NFT loss. This process introduces unnecessary risks, and the protocol should implement a more elegant solution.
Vulnerability Details
ERC721's _transfer() requires 'from' and 'owner' to be identical. However, the L1 depositTokens() directly treats msg.sender as both the NFT's 'from' and 'owner', failing to consider the possibility of users authorizing contracts for cross-chain operations. If an authorized protocol forcibly implements cross-chain operations by first transferring the NFT to its own contract, req.ownerL1 will be registered as the authorized protocol. In case of a cancelRequest(), firstly, the authorized protocol may not meet ERC721's safeTransferFrom() conditions; secondly, even if the refund is completed, the NFT ownership would transfer to the authorized protocol rather than the original user. Both scenarios increase the risk of users losing their NFTs.
POC
This test will attempt to interact with the Bridge using an authorized contract. We will test three scenarios:
The authorized contract attempts to initiate cross-chain transfer without ERC721 ownership, which should fail.
The authorized contract first obtains ERC721 ownership and then initiates cross-chain transfer. This can successfully submit, but if the cross-chain transfer is cancelled, the Bridge will return the ERC721 to the authorized contract, causing the original user to lose ERC721 ownership.
Improve the Bridge implementation so that the authorized contract can help users initiate cross-chain transfer with just approval. If cancelled, the ERC721 can be returned to the original owner.
Create a file ApprovalContract.t.sol in the test/mock folder:
pragma solidity ^0.8.0;
import "../../src/sn/IStarknetMessagingLocal.sol";
import "../../src/IStarklane.sol";
import "../../src/sn/Cairo.sol";
import "openzeppelin-contracts/contracts/token/ERC721/IERC721.sol";
import "openzeppelin-contracts/contracts/token/ERC721/IERC721Receiver.sol";
import {Test, console} from "forge-std/Test.sol";
contract ApprovalContract is Test, IERC721Receiver {
address bridge;
address erc721C1;
constructor(address _bridge, address _erc721C1) {
bridge = _bridge;
erc721C1 = _erc721C1;
}
function depositTokensWithoutERC721(uint256 salt, address collectionL1, snaddress ownerL2, uint256[] calldata ids, bool useAutoBurn) public {
IStarklane(bridge).depositTokens{value: 30000}(
salt,
collectionL1,
ownerL2,
ids,
useAutoBurn
);
}
function depositTokensAfterGetERC721(uint256 salt, address collectionL1, snaddress ownerL2, uint256[] calldata ids, bool useAutoBurn) public {
uint256 length = ids.length;
for (uint256 i = 0; i < length; i++) {
IERC721(erc721C1).transferFrom(msg.sender, address(this), ids[i]);
}
IERC721(erc721C1).setApprovalForAll(bridge, true);
IStarklane(bridge).depositTokens{value: 30000}(
salt,
collectionL1,
ownerL2,
ids,
useAutoBurn
);
}
function depositTokensBetter(uint256 salt, address collectionL1, snaddress ownerL2, uint256[] calldata ids, bool useAutoBurn) public {
IStarklane(bridge).depositTokensBetter{value: 30000}(
salt,
collectionL1,
ownerL2,
ids,
useAutoBurn
);
}
function onERC721Received(
address operator,
address from,
uint256 tokenId,
bytes calldata data
) external override returns (bytes4) {
return this.onERC721Received.selector;
}
}
In test/Bridge.sol, add the following code:
import "./mock/approvalContract.t.sol";
function test_cancelRequest_forApprovalContract() public {
uint256[] memory ids = new uint256[](2);
ids[0] = 0;
ids[1] = 9;
ApprovalContract c = new ApprovalContract(bridge, erc721C1);
vm.deal(address(c), 10 ether);
* selector options:
* ApprovalContract.depositTokensWithoutERC721.selector: Fail - Contract is not the ERC721 Owner, cannot transfer
* ApprovalContract.depositTokensAfterGetERC721.selector: Pass - However, during and after this process, ERC721 ownership belongs to the contract, not the original owner
* ApprovalContract.depositTokensBetter.selector: Pass - Authorized contract can help users cross-chain transfer without holding ERC721 ownership and successfully return
*/
bytes4 selector = ApprovalContract.depositTokensBetter.selector;
address user = alice;
(uint256 nonce, uint256[] memory payload) = setupCancelRequestForApprovalContract(user, address(c), selector, ids);
assert(IERC721(erc721C1).ownerOf(ids[0]) != alice);
assert(IERC721(erc721C1).ownerOf(ids[1]) != alice);
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);
if (selector == ApprovalContract.depositTokensBetter.selector) {
assert(IERC721(erc721C1).ownerOf(ids[0]) == address(user));
assert(IERC721(erc721C1).ownerOf(ids[1]) == address(user));
} else {
assert(IERC721(erc721C1).ownerOf(ids[0]) == address(c));
assert(IERC721(erc721C1).ownerOf(ids[1]) == address(c));
}
}
function setupCancelRequestForApprovalContract(
address user,
address approvalContract,
bytes4 selector,
uint256[] memory tokenIds
) internal returns(uint256, uint256[] memory) {
IERC721MintRangeFree(erc721C1).mintRangeFree(user, 0, 10);
uint256 salt = 0x1;
snaddress to = Cairo.snaddressWrap(0x1);
bytes memory callData = abi.encode(
salt,
address(erc721C1),
to,
tokenIds,
false
);
vm.startPrank(user);
IERC721(erc721C1).setApprovalForAll(approvalContract, true);
IERC721(erc721C1).setApprovalForAll(bridge, true);
vm.recordLogs();
(bool success, ) = approvalContract.call(abi.encodePacked(selector, callData));
require(success, "Function call failed");
Vm.Log[] memory entries = vm.getRecordedLogs();
vm.stopPrank();
Vm.Log memory logMessageToL2;
Vm.Log memory depositRequestEvent;
if (selector == ApprovalContract.depositTokensAfterGetERC721.selector) {
assertEq(entries.length, 7);
logMessageToL2 = entries[5];
depositRequestEvent = entries[6];
} else {
assertEq(entries.length, 4);
logMessageToL2 = entries[2];
depositRequestEvent = entries[3];
}
(uint256[] memory payload, uint256 nonce, ) = abi.decode(logMessageToL2.data, (uint256[], uint256, uint256));
( ,uint256[] memory reqContent) = abi.decode(depositRequestEvent.data, (uint256, uint256[]));
assert(payload.length == reqContent.length);
return (nonce, payload);
}
You can use different selectors in test_cancelRequest_forApprovalContract() to test different scenarios.
In src/Bridge.sol, add the following code:
function depositTokensBetter(
uint256 salt,
address collectionL1,
snaddress ownerL2,
uint256[] calldata ids,
bool useAutoBurn
)
external
payable
{
if (!Cairo.isFelt252(snaddress.unwrap(ownerL2))) {
revert CairoWrapError();
}
if (!_enabled) {
revert BridgeNotEnabledError();
}
CollectionType ctype = TokenUtil.detectInterface(collectionL1);
if (ctype == CollectionType.ERC1155) {
revert NotSupportedYetError();
}
if (!_isWhiteListed(collectionL1)) {
revert NotWhiteListedError();
}
Request memory req;
req.header = Protocol.requestHeaderV1(ctype, useAutoBurn, false);
req.hash = Protocol.requestHash(salt, collectionL1, ownerL2, ids);
req.collectionL1 = collectionL1;
req.collectionL2 = _l1ToL2Addresses[collectionL1];
address ownerL1 = IERC721(collectionL1).ownerOf(ids[0]);
uint256 length = ids.length;
for (uint256 i = 1; i < length; i++) {
require(ownerL1 == IERC721(collectionL1).ownerOf(ids[i]), "Different Owner");
}
address sender = msg.sender;
if (ownerL1 == sender || IERC721(collectionL1).isApprovedForAll(ownerL1, sender)) {}
else {
for (uint256 i = 0; i < length; i++) {
require(IERC721(collectionL1).getApproved(ids[i]) == sender, "sender doesn't get the approved");
}
}
req.ownerL1 = ownerL1;
req.ownerL2 = ownerL2;
if (ctype == CollectionType.ERC721) {
(req.name, req.symbol, req.uri, req.tokenURIs) = TokenUtil.erc721Metadata(
collectionL1,
ids
);
} else {
(req.uri) = TokenUtil.erc1155Metadata(collectionL1);
}
_depositIntoEscrowBetter(ctype, collectionL1, ownerL1, ids);
req.tokenIds = ids;
uint256[] memory payload = Protocol.requestSerialize(req);
if (payload.length >= MAX_PAYLOAD_LENGTH) {
revert TooManyTokensError();
}
IStarknetMessaging(_starknetCoreAddress).sendMessageToL2{value: msg.value}(
snaddress.unwrap(_starklaneL2Address),
felt252.unwrap(_starklaneL2Selector),
payload
);
emit DepositRequestInitiated(req.hash, block.timestamp, payload);
}
In src/Escrow.sol, add the following code:
function _depositIntoEscrowBetter(
CollectionType collectionType,
address collection,
address ownerL1,
uint256[] memory ids
)
internal
{
assert(ids.length > 0);
for (uint256 i = 0; i < ids.length; i++) {
uint256 id = ids[i];
if (collectionType == CollectionType.ERC721) {
IERC721(collection).transferFrom(ownerL1, address(this), id);
} else {
IERC1155(collection).safeTransferFrom(ownerL1, address(this), id, 1, "");
}
_escrow[collection][id] = ownerL1;
}
}
When testing with ApprovalContract.depositTokensWithoutERC721.selector to test the case where the authorized contract doesn't hold ERC721, the following output will be produced:
..........
│ │ │ ├─ [1606] ERC721MintFree::transferFrom(ApprovalContract: [0xa0Cb889707d426A7A386870A03bc70d1b0697598], ERC1967Proxy: [0xc7183455a4C133Ae270771860664b6B7ec320bB1], 0)
│ │ │ │ └─ ← [Revert] revert: ERC721: transfer from incorrect owner
│ │ │ └─ ← [Revert] revert: ERC721: transfer from incorrect owner
│ │ └─ ← [Revert] revert: ERC721: transfer from incorrect owner
│ └─ ← [Revert] revert: ERC721: transfer from incorrect owner
└─ ← [Revert] revert: Function call failed
ApprovalContract attempts to transfer the user's ERC721 during the depositTokens() process, but since it's not the Owner, the transfer fails.
When testing with ApprovalContract.depositTokensAfterGetERC721.selector to test the case where the authorized contract first holds ERC721, the following output will be produced:
│ │ ├─ emit CancelRequestCompleted(hash: 44533365631490511846136959232473399416663084422311991968200612851427666414595 [4.453e76], block_timestamp: 1)
│ │ └─ ← [Stop]
│ └─ ← [Return]
├─ [602] ERC721MintFree::ownerOf(0) [staticcall]
│ └─ ← [Return] ApprovalContract: [0xa0Cb889707d426A7A386870A03bc70d1b0697598]
├─ [602] ERC721MintFree::ownerOf(9) [staticcall]
│ └─ ← [Return] ApprovalContract: [0xa0Cb889707d426A7A386870A03bc70d1b0697598]
After cancelling the cross-chain transfer, the ERC721 ownership belongs to the authorized contract, and the user loses ownership.
When testing with ApprovalContract.depositTokensBetter.selector to test cross-chain transfer with just approval and successfully returning to the user:
│ │ ├─ emit CancelRequestCompleted(hash: 44533365631490511846136959232473399416663084422311991968200612851427666414595 [4.453e76], block_timestamp: 1)
│ │ └─ ← [Stop]
│ └─ ← [Return]
├─ [602] ERC721MintFree::ownerOf(0) [staticcall]
│ └─ ← [Return] 0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7
├─ [602] ERC721MintFree::ownerOf(9) [staticcall]
│ └─ ← [Return] 0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7
Impact
Protocols wishing to interact with ArkProject may abandon the idea due to the high risks involved, limiting ArkProject's interoperability and scalability. Even if some protocols are willing to proceed, it introduces significant uncertainties for the future operations of both parties.
Tools Used
Manual Review
Recommendations
The key issue is that the protocol shouldn't assume all interacting parties are user accounts and definitely own the NFT. In depositTokens(), when constructing the request, both ownerL1 and 'from' in transferFrom should use ownerOf(tokenId) instead of msg.sender, and _isApprovedOrOwner() should be used to check the caller's permissions.You can refer to the implementation of depositTokensBetter() and _depositIntoEscrowBetter() in the POC for guidance.
