Summary
When withdrawTokens is called and collectionL1 returns a non-zero value, it is possible that the collectionL1 has been removed from the whitelist. However, the whitelist status of collectionL1 is not checked, allowing the users to still interact with non-whitelisted collections.
Vulnerability Details
Bridge owner can add list of collections that is whitelisted, means only those collections that are allowed to bridged by users when _whiteListEnabled is set to true.
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Bridge.sol#L284-L287
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Bridge.sol#L340-L356
function whiteList(address collection, bool enable) external onlyOwner {
_whiteListCollection(collection, enable);
emit CollectionWhiteListUpdated(collection, enable);
}
function _whiteListCollection(address collection, bool enable) internal {
if (enable && !_whiteList[collection]) {
bool toAdd = true;
uint256 i = 0;
while(i < _collections.length) {
if (collection == _collections[i]) {
toAdd = false;
break;
}
i++;
}
if (toAdd) {
_collections.push(collection);
}
}
_whiteList[collection] = enable;
}
However, when withdrawTokens is triggered by users and and collectionL1 returns a non-zero value from _verifyRequestAddresses, it is possible that the collectionL1 is already removed from whitelist due to various reasons. However, the whitelist status of collectionL1 is not checked, allowing the users to still interact with non-whitelisted collections.
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Bridge.sol#L183-L210
function withdrawTokens(
uint256[] calldata request
)
external
payable
returns (address)
{
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
This means collectionL1 that is removed from whitelist, users can still interact with it even when _whiteListEnabled is set to true.
Tools Used
Manual review
Recommendations
Consider to check if collectionL1 is whitelisted when the value is non-zero.
function withdrawTokens(
uint256[] calldata request
)
external
payable
returns (address)
{
// ...
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();
}
- }
+ } else {
+ 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;
}
