From 07c3164a396bb3299b1842051d7bf36807ea1900 Mon Sep 17 00:00:00 2001 From: enitrat Date: Mon, 4 Nov 2024 14:36:43 +0700 Subject: [PATCH] fix: add NoDelegateCall modifiers to DualVMToken --- .../src/CairoPrecompiles/DualVmToken.sol | 25 +-- .../src/L1L2Messaging/L2KakarotMessaging.sol | 8 +- .../src/Security/DualVmTokenHack.sol | 80 +++++++++ .../src/Security/L2KakarotMessagingHack.sol | 18 ++ .../src/Security/NoDelegateCall.sol | 28 ++++ .../Security/test_dual_vm_token_hack.py | 154 ++++++++++++++++++ .../Security/test_l2_messaging_hack.py | 34 ++++ 7 files changed, 335 insertions(+), 12 deletions(-) create mode 100644 solidity_contracts/src/Security/DualVmTokenHack.sol create mode 100644 solidity_contracts/src/Security/L2KakarotMessagingHack.sol create mode 100644 solidity_contracts/src/Security/NoDelegateCall.sol create mode 100644 tests/end_to_end/Security/test_dual_vm_token_hack.py create mode 100644 tests/end_to_end/Security/test_l2_messaging_hack.py diff --git a/solidity_contracts/src/CairoPrecompiles/DualVmToken.sol b/solidity_contracts/src/CairoPrecompiles/DualVmToken.sol index 3f1075237..22aa3c7b2 100644 --- a/solidity_contracts/src/CairoPrecompiles/DualVmToken.sol +++ b/solidity_contracts/src/CairoPrecompiles/DualVmToken.sol @@ -1,16 +1,21 @@ -// SPDX-License-Identifier: AGPL-3.0-only +// SPDX-License-Identifier: MIT pragma solidity 0.8.27; import {WhitelistedCallCairoLib} from "./WhitelistedCallCairoLib.sol"; import {CairoLib} from "kakarot-lib/CairoLib.sol"; +import {NoDelegateCall} from "../Security/NoDelegateCall.sol"; /// @notice EVM adapter into a Cairo ERC20 token /// @dev This implementation is highly experimental /// It relies on CairoLib to perform Cairo precompile calls /// Events are emitted in this contract but also in the Starknet token contract +/// @dev External functions are noDelegateCall to prevent a user making an EVM call to a malicious contract, +/// with any calldata, that would be able to directly control on their behalf any quantity of any one of the ERC20 +/// tokens held by the victim's account contract, with the sole condition that the ERC20 has an +/// authorized DualVmToken wrapper. /// @author Kakarot /// @author Modified from Solmate (https://github.com/transmissions11/solmate/blob/main/src/tokens/ERC20.sol) -contract DualVmToken { +contract DualVmToken is NoDelegateCall { using WhitelistedCallCairoLib for uint256; /*////////////////////////////////////////////////////////////// CAIRO SPECIFIC VARIABLES @@ -204,7 +209,7 @@ contract DualVmToken { //////////////////////////////////////////////////////////////*/ /// @dev Approve an evm account spender for a specific amount - function approve(address spender, uint256 amount) external returns (bool) { + function approve(address spender, uint256 amount) external noDelegateCall returns (bool) { uint256[] memory spenderAddressCalldata = new uint256[](1); spenderAddressCalldata[0] = uint256(uint160(spender)); uint256 spenderStarknetAddress = @@ -220,7 +225,7 @@ contract DualVmToken { /// @param spender The starknet address to approve /// @param amount The amount of tokens to approve /// @return True if the approval was successful - function approve(uint256 spender, uint256 amount) external returns (bool) { + function approve(uint256 spender, uint256 amount) external noDelegateCall returns (bool) { _approve(spender, amount); emit Approval(msg.sender, spender, amount); return true; @@ -245,7 +250,7 @@ contract DualVmToken { /// @param to The evm address to transfer the tokens to /// @param amount The amount of tokens to transfer /// @return True if the transfer was successful - function transfer(address to, uint256 amount) external returns (bool) { + function transfer(address to, uint256 amount) external noDelegateCall returns (bool) { uint256[] memory toAddressCalldata = new uint256[](1); toAddressCalldata[0] = uint256(uint160(to)); uint256 toStarknetAddress = @@ -260,7 +265,7 @@ contract DualVmToken { /// @param to The starknet address to transfer the tokens to /// @param amount The amount of tokens to transfer /// @return True if the transfer was successful - function transfer(uint256 to, uint256 amount) external returns (bool) { + function transfer(uint256 to, uint256 amount) external noDelegateCall returns (bool) { _transfer(to, amount); emit Transfer(msg.sender, to, amount); return true; @@ -287,7 +292,7 @@ contract DualVmToken { /// @param to The evm address to transfer the tokens to /// @param amount The amount of tokens to transfer /// @return True if the transfer was successful - function transferFrom(address from, address to, uint256 amount) external returns (bool) { + function transferFrom(address from, address to, uint256 amount) external noDelegateCall returns (bool) { uint256[] memory toAddressCalldata = new uint256[](1); toAddressCalldata[0] = uint256(uint160(to)); uint256 toStarknetAddress = @@ -309,7 +314,7 @@ contract DualVmToken { /// @param to The evm address to transfer the tokens to /// @param amount The amount of tokens to transfer /// @return True if the transfer was successful - function transferFrom(uint256 from, address to, uint256 amount) external returns (bool) { + function transferFrom(uint256 from, address to, uint256 amount) external noDelegateCall returns (bool) { uint256[] memory toAddressCalldata = new uint256[](1); toAddressCalldata[0] = uint256(uint160(to)); uint256 toStarknetAddress = @@ -326,7 +331,7 @@ contract DualVmToken { /// @param to The starknet address to transfer the tokens to /// @param amount The amount of tokens to transfer /// @return True if the transfer was successful - function transferFrom(address from, uint256 to, uint256 amount) external returns (bool) { + function transferFrom(address from, uint256 to, uint256 amount) external noDelegateCall returns (bool) { uint256[] memory fromAddressCalldata = new uint256[](1); fromAddressCalldata[0] = uint256(uint160(from)); uint256 fromStarknetAddress = @@ -343,7 +348,7 @@ contract DualVmToken { /// @param to The starknet address to transfer the tokens to /// @param amount The amount of tokens to transfer /// @return True if the transfer was successful - function transferFrom(uint256 from, uint256 to, uint256 amount) external returns (bool) { + function transferFrom(uint256 from, uint256 to, uint256 amount) external noDelegateCall returns (bool) { _transferFrom(from, to, amount); emit Transfer(from, to, amount); return true; diff --git a/solidity_contracts/src/L1L2Messaging/L2KakarotMessaging.sol b/solidity_contracts/src/L1L2Messaging/L2KakarotMessaging.sol index 20a2947f8..2018aab02 100644 --- a/solidity_contracts/src/L1L2Messaging/L2KakarotMessaging.sol +++ b/solidity_contracts/src/L1L2Messaging/L2KakarotMessaging.sol @@ -2,12 +2,16 @@ pragma solidity 0.8.27; import {CairoLib} from "kakarot-lib/CairoLib.sol"; +import {NoDelegateCall} from "../Security/NoDelegateCall.sol"; -contract L2KakarotMessaging { +contract L2KakarotMessaging is NoDelegateCall { /// @notice Sends a message to a contract on L1. + /// @dev This function is noDelegateCall to prevent attack vectors where a + /// contract can send messages to L1 with arbitrary target addresses and payloads; + /// these messages appear as originated by victim's EVM address. /// @param to The address of the contract on L1 to send the message to. /// @param data The data to send to the contract on L1. - function sendMessageToL1(address to, bytes calldata data) external { + function sendMessageToL1(address to, bytes calldata data) external noDelegateCall { bytes memory payload = abi.encode(to, msg.sender, data); CairoLib.sendMessageToL1(payload); } diff --git a/solidity_contracts/src/Security/DualVmTokenHack.sol b/solidity_contracts/src/Security/DualVmTokenHack.sol new file mode 100644 index 000000000..c252bd636 --- /dev/null +++ b/solidity_contracts/src/Security/DualVmTokenHack.sol @@ -0,0 +1,80 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.27; + +contract DualVmTokenHack { + address target; + uint256 constant AMOUNT = 1337; + + constructor(address _target) { + target = _target; + } + + function tryApproveEvm() external returns (bool success) { + (success,) = target.delegatecall( + abi.encodeWithSelector(bytes4(keccak256("approve(address,uint256)")), address(this), AMOUNT) + ); + } + + function tryApproveStarknet() external returns (bool success) { + (success,) = target.delegatecall( + abi.encodeWithSelector( + bytes4(keccak256("approve(uint256,uint256)")), uint256(uint160(address(this))), AMOUNT + ) + ); + } + + function tryTransferEvm() external returns (bool success) { + (success,) = target.delegatecall( + abi.encodeWithSelector(bytes4(keccak256("transfer(address,uint256)")), address(this), AMOUNT) + ); + } + + function tryTransferStarknet() external returns (bool success) { + (success,) = target.delegatecall( + abi.encodeWithSelector( + bytes4(keccak256("transfer(uint256,uint256)")), uint256(uint160(address(this))), AMOUNT + ) + ); + } + + function tryTransferFromEvmEvm() external returns (bool success) { + (success,) = target.delegatecall( + abi.encodeWithSelector( + bytes4(keccak256("transferFrom(address,address,uint256)")), msg.sender, address(this), AMOUNT + ) + ); + } + + function tryTransferFromStarknetEvm() external returns (bool success) { + (success,) = target.delegatecall( + abi.encodeWithSelector( + bytes4(keccak256("transferFrom(uint256,address,uint256)")), + uint256(uint160(msg.sender)), + address(this), + AMOUNT + ) + ); + } + + function tryTransferFromEvmStarknet() external returns (bool success) { + (success,) = target.delegatecall( + abi.encodeWithSelector( + bytes4(keccak256("transferFrom(address,uint256,uint256)")), + msg.sender, + uint256(uint160(address(this))), + AMOUNT + ) + ); + } + + function tryTransferFromStarknetStarknet() external returns (bool success) { + (success,) = target.delegatecall( + abi.encodeWithSelector( + bytes4(keccak256("transferFrom(uint256,uint256,uint256)")), + uint256(uint160(msg.sender)), + uint256(uint160(address(this))), + AMOUNT + ) + ); + } +} diff --git a/solidity_contracts/src/Security/L2KakarotMessagingHack.sol b/solidity_contracts/src/Security/L2KakarotMessagingHack.sol new file mode 100644 index 000000000..e5b4200b9 --- /dev/null +++ b/solidity_contracts/src/Security/L2KakarotMessagingHack.sol @@ -0,0 +1,18 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.27; + +import {L2KakarotMessaging} from "../L1L2Messaging/L2KakarotMessaging.sol"; + +contract L2MessagingHack { + L2KakarotMessaging public immutable target; + + constructor(address _target) { + target = L2KakarotMessaging(_target); + } + + function trySendMessageToL1(address to, bytes calldata data) external returns (bool success) { + // Try to send message through delegatecall + (success,) = + address(target).delegatecall(abi.encodeWithSelector(L2KakarotMessaging.sendMessageToL1.selector, to, data)); + } +} diff --git a/solidity_contracts/src/Security/NoDelegateCall.sol b/solidity_contracts/src/Security/NoDelegateCall.sol new file mode 100644 index 000000000..31032d30a --- /dev/null +++ b/solidity_contracts/src/Security/NoDelegateCall.sol @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: BUSL-1.1 +// Author: Uniswap Labs https://github.com/Uniswap/v3-core/blob/main/contracts/NoDelegateCall.sol +pragma solidity 0.8.27; + +/// @title Prevents delegatecall to a contract +/// @notice Base contract that provides a modifier for preventing delegatecall to methods in a child contract +abstract contract NoDelegateCall { + /// @dev The original address of this contract + address private immutable original; + + constructor() { + // Immutables are computed in the init code of the contract, and then inlined into the deployed bytecode. + // In other words, this variable won't change when it's checked at runtime. + original = address(this); + } + + /// @dev Private method is used instead of inlining into modifier because modifiers are copied into each method, + /// and the use of immutable means the address bytes are copied in every place the modifier is used. + function checkNotDelegateCall() private view { + require(address(this) == original); + } + + /// @notice Prevents delegatecall into the modified method + modifier noDelegateCall() { + checkNotDelegateCall(); + _; + } +} diff --git a/tests/end_to_end/Security/test_dual_vm_token_hack.py b/tests/end_to_end/Security/test_dual_vm_token_hack.py new file mode 100644 index 000000000..673c4dd9b --- /dev/null +++ b/tests/end_to_end/Security/test_dual_vm_token_hack.py @@ -0,0 +1,154 @@ +import logging + +import pytest +import pytest_asyncio + +from kakarot_scripts.utils.kakarot import deploy as deploy_kakarot +from kakarot_scripts.utils.starknet import deploy as deploy_starknet +from kakarot_scripts.utils.starknet import get_contract as get_contract_starknet +from kakarot_scripts.utils.starknet import invoke + +logging.basicConfig() +logger = logging.getLogger(__name__) +logger.setLevel(logging.INFO) + + +@pytest_asyncio.fixture(scope="function") +async def starknet_token(owner): + address = await deploy_starknet( + "StarknetToken", int(1e18), owner.starknet_contract.address + ) + return get_contract_starknet("StarknetToken", address=address) + + +@pytest_asyncio.fixture(scope="function") +async def dual_vm_token(kakarot, starknet_token, owner): + dual_vm_token = await deploy_kakarot( + "CairoPrecompiles", + "DualVmToken", + kakarot.address, + starknet_token.address, + caller_eoa=owner.starknet_contract, + ) + + await invoke( + "kakarot", + "set_authorized_cairo_precompile_caller", + int(dual_vm_token.address, 16), + True, + ) + return dual_vm_token + + +@pytest_asyncio.fixture(scope="function") +async def hack_vm_token(dual_vm_token, owner): + hack_vm_token = await deploy_kakarot( + "CairoPrecompiles", + "DualVmTokenHack", + dual_vm_token.address, + caller_eoa=owner.starknet_contract, + ) + return hack_vm_token + + +@pytest.mark.asyncio(scope="module") +@pytest.mark.CairoPrecompiles +class TestDualVmToken: + class TestActions: + async def test_malicious_approve_address_should_fail_nodelegatecaltest_malicious_approve_address_should_fail_nodelegatecall( + self, dual_vm_token, hack_vm_token, owner + ): + result = await hack_vm_token.functions["tryApproveEvm()"]() + call_succeeded = int.from_bytes(bytes(result["response"]), "big") + assert call_succeeded == 0 + + allowance = await dual_vm_token.functions["allowance(address,address)"]( + owner.address, hack_vm_token.address + ) + assert allowance == 0 + + async def test_malicious_approve_starknet_should_fail_nodelegatecall( + self, dual_vm_token, hack_vm_token, owner + ): + result = await hack_vm_token.functions["tryApproveStarknet()"]() + call_succeeded = int.from_bytes(bytes(result["response"]), "big") + assert call_succeeded == 0 + + allowance = await dual_vm_token.functions["allowance(address,uint256)"]( + owner.address, int(hack_vm_token.address, 16) + ) + assert allowance == 0 + + async def test_malicious_transfer_address_should_fail_nodelegatecall( + self, dual_vm_token, hack_vm_token, owner + ): + result = await hack_vm_token.functions["tryTransferEvm()"]() + call_succeeded = int.from_bytes(bytes(result["response"]), "big") + assert call_succeeded == 0 + + balance = await dual_vm_token.functions["balanceOf(address)"]( + hack_vm_token.address + ) + assert balance == 0 + + async def test_malicious_transfer_starknet_should_fail_nodelegatecall( + self, dual_vm_token, hack_vm_token, owner + ): + result = await hack_vm_token.functions["tryTransferStarknet()"]() + call_succeeded = int.from_bytes(bytes(result["response"]), "big") + assert call_succeeded == 0 + + balance = await dual_vm_token.functions["balanceOf(uint256)"]( + int(hack_vm_token.address, 16) + ) + assert balance == 0 + + async def test_malicious_transfer_from_address_address_should_fail_nodelegatecall( + self, dual_vm_token, hack_vm_token, owner + ): + result = await hack_vm_token.functions["tryTransferFromEvmEvm()"]() + call_succeeded = int.from_bytes(bytes(result["response"]), "big") + assert call_succeeded == 0 + + balance = await dual_vm_token.functions["balanceOf(address)"]( + hack_vm_token.address + ) + assert balance == 0 + + async def test_malicious_transfer_from_starknet_address_should_fail_nodelegatecall( + self, dual_vm_token, hack_vm_token, owner + ): + result = await hack_vm_token.functions["tryTransferFromStarknetEvm()"]() + call_succeeded = int.from_bytes(bytes(result["response"]), "big") + assert call_succeeded == 0 + + balance = await dual_vm_token.functions["balanceOf(address)"]( + hack_vm_token.address + ) + assert balance == 0 + + async def test_malicious_transfer_from_address_starknet_should_fail_nodelegatecall( + self, dual_vm_token, hack_vm_token, owner + ): + result = await hack_vm_token.functions["tryTransferFromEvmStarknet()"]() + call_succeeded = int.from_bytes(bytes(result["response"]), "big") + assert call_succeeded == 0 + + balance = await dual_vm_token.functions["balanceOf(uint256)"]( + int(hack_vm_token.address, 16) + ) + assert balance == 0 + + async def test_malicious_transfer_from_starknet_starknet_should_fail_nodelegatecall( + self, dual_vm_token, hack_vm_token, owner + ): + result = await hack_vm_token.functions[ + "tryTransferFromStarknetStarknet()" + ]() + call_succeeded = int.from_bytes(bytes(result["response"]), "big") + assert call_succeeded == 0 + + balance = await dual_vm_token.functions["balanceOf(uint256)"]( + int(hack_vm_token.address, 16) + ) + assert balance == 0 diff --git a/tests/end_to_end/Security/test_l2_messaging_hack.py b/tests/end_to_end/Security/test_l2_messaging_hack.py new file mode 100644 index 000000000..2050fd49f --- /dev/null +++ b/tests/end_to_end/Security/test_l2_messaging_hack.py @@ -0,0 +1,34 @@ +import pytest +import pytest_asyncio +from eth_utils.address import to_checksum_address + +from kakarot_scripts.utils.kakarot import deploy +from kakarot_scripts.utils.kakarot import get_deployments as get_evm_deployments + + +@pytest_asyncio.fixture(scope="function") +async def messaging_hack_contract(owner): + return await deploy( + "Security", + "L2MessagingHack", + _target=to_checksum_address( + get_evm_deployments()["L2KakarotMessaging"]["address"] + ), + caller_eoa=owner.starknet_contract, + ) + + +@pytest.mark.asyncio(scope="module") +class TestL2MessagingHack: + async def test_malicious_message_should_fail_nodelegatecall( + self, + messaging_hack_contract, + ): + malicious_target = "0x1234567890123456789012345678901234567890" + malicious_data = "0xdeadbeef" + + result = await messaging_hack_contract.functions[ + "trySendMessageToL1(address,bytes)" + ](malicious_target, malicious_data) + call_succeeded = int.from_bytes(bytes(result["response"]), "big") + assert call_succeeded == 0