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

ownerL2 not verified against zero address when sending from L1 to L2

crunter

Summary

There is no check against zero address for L2 owner when sending from L1 to L2. In the worst case, the token bridged from L1 is held by the bridge in L2 as escrow. If L2 owner is the zero address and an user executes the withdrawal function on L2 for that request, the token will be transferred from the L2 bridge to the zero address, i. e., the token will be burned.

Vulnerability Details

test/Bridge.t.sol::testFail_invalidL2Owner claims to check for the existence of a valid ownerL2 address. However, the test passes because the assert(ids.length > 0); statement in StarklaneEscrow::_depositIntoEscrow function fails and no actual verification of the ownerL2 address is ever performed.

Proof of Concept

Add the following test to the Bridge.t.sol file and execute it.

function test_revertWithInvalidL2Owner_audit() public {
uint256[] memory somePayload = new uint256[](0);
uint256 someHash;
​
// To avoid the fail of assert(ids.length > 0) statement
IERC721MintRangeFree(erc721C1).mintRangeFree(alice, 0, 10);
​
uint256[] memory ids = new uint256[](2);
ids[0] = 0;
ids[1] = 9;
​
uint256 salt = 0x0;
snaddress to = Cairo.snaddressWrap(0x0);
​
vm.startPrank(alice);
IERC721(erc721C1).setApprovalForAll(address(bridge), true);
​
// DepositRequetInitiated event should not be emitted as
// L2 owner address is invalid. If test pass,
// L2 owner address will be set to the zero address in the request
vm.expectEmit(false, false, false, false); // Unnecessary to check the indexed values
emit DepositRequestInitiated(someHash, 1, somePayload);
​
IStarklane(bridge).depositTokens{value: 30000}(salt, address(erc721C1), to, ids, false);
vm.stopPrank();
}

Test passes because of the lack of L2 owner address verification.

Impact

Impact: High in the worst scenario

Likelihood: Low

Tools Used

Manual Review

Recommendations

Add a verification for the L2 owner address in the Starklane::depositTokens function:

...
+ error ZeroAddressError();
...
function depositTokens(
uint256 salt,
address collectionL1,
snaddress ownerL2,
uint256[] calldata ids,
bool useAutoBurn
)
external
payable
{
+ if(address(uint160(snaddress.unwrap(ownerL2))) == address(0)) revert ZeroAddressError();
...
}
Updates

Lead Judging Commences

n0kto Lead Judge 11 months 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.