Summary
Transfer from L2 → L1 does not check if the L1 collection is whitelisted.
Vulnerability Details
When tokens are transferred from L2 —> L1 and collections exist on both chains, on L1 inside withdrawTokens() there is no check if the collection is still whitelisted as was done on L2.
The way that unwhitelisting works is that admins should only remove the token from the _whiteList mapping:
mapping(address => bool) _whiteList;
l1 ↔ l2 mappings should remain untouched, otherwise that will deploy a new token and whitelist it automatically, but the admins wanted to completely remove the possibility of withdrawing this token and that is why only _whitelist is being modified, then when withdrawTokens is called since the check is missing we will be able to retrieve unwhitelisted tokens:
function withdrawTokens(
uint256[] calldata request
)
external
payable
returns (address)
{
if (!_enabled) {
revert BridgeNotEnabledError();
}
uint256 header = request[0];
if (Protocol.canUseWithdrawAuto(header)) {
revert NotSupportedYetError();
} else {
_consumeMessageStarknet(_starknetCoreAddress, _starklaneL2Address, request);
}
Request memory req = Protocol.requestDeserialize(request, 0);
address collectionL1 = _verifyRequestAddresses(req.collectionL1, req.collectionL2);
CollectionType ctype = Protocol.collectionTypeFromHeader(header);
if (collectionL1 == address(0x0)) {
if (ctype == CollectionType.ERC721) {
collectionL1 = _deployERC721Bridgeable(
req.name,
req.symbol,
req.collectionL2,
req.hash
);
_whiteListCollection(collectionL1, true);
} else {
revert NotSupportedYetError();
}
}
for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];
bool wasEscrowed = _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
if (!wasEscrowed) {
IERC721Bridgeable(collectionL1).mintFromBridge(req.ownerL1, id);
}
}
emit WithdrawRequestCompleted(req.hash, block.timestamp, request);
return collectionL1;
}
Impact
There is no check if the collection is still in the L1 whitelist, which would allow tokens to be obtained from a collection that is not permitted for use by the protocol.
Tools Used
Manual
Recommendations
function withdrawTokens(
uint256[] calldata request
)
external
payable
returns (address)
{
if (!_enabled) {
revert BridgeNotEnabledError();
}
// Header is always the first uint256 of the serialized request.
uint256 header = request[0];
// Any error or permission fail in the message consumption will cause a revert.
// After message being consumed, it is considered legit and tokens can be withdrawn.
if (Protocol.canUseWithdrawAuto(header)) {
// 2024-03-19: disabled autoWithdraw after audit report
// _consumeMessageAutoWithdraw(_starklaneL2Address, request);
revert NotSupportedYetError();
} else {
_consumeMessageStarknet(_starknetCoreAddress, _starklaneL2Address, request);
}
Request memory req = Protocol.requestDeserialize(request, 0);
address collectionL1 = _verifyRequestAddresses(req.collectionL1, req.collectionL2);
CollectionType ctype = Protocol.collectionTypeFromHeader(header);
if (collectionL1 == address(0x0)) {
if (ctype == CollectionType.ERC721) {
collectionL1 = _deployERC721Bridgeable(
req.name,
req.symbol,
req.collectionL2,
req.hash
);
// update whitelist if needed
_whiteListCollection(collectionL1, true);
} else {
revert NotSupportedYetError();
}
}
+ if (!_isWhiteListed(collectionL1)) {
+ revert NotWhiteListedError();
+ }
for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];
bool wasEscrowed = _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
if (!wasEscrowed) {
// TODO: perhaps, implement the same interface for ERC721 and ERC1155
// As we only want to deal with ERC1155 token with value = 1.
// Also, check what to do with URIs. If the URI storage is supported
// or not for ERC721. If supported, we may need to mint with an URI.
IERC721Bridgeable(collectionL1).mintFromBridge(req.ownerL1, id);
}
}
emit WithdrawRequestCompleted(req.hash, block.timestamp, request);
return collectionL1;
}
