From c71f8f3c074b4dc3bf9608caf8d389cddd5c7775 Mon Sep 17 00:00:00 2001 From: pscott <30843220+pscott@users.noreply.github.com> Date: Thu, 20 Oct 2022 16:14:49 +0200 Subject: [PATCH] Make sure all uint256 are valid (#370) * Make sure all uint256 are valid * Fix compilaton of felt_utils --- .../starknet/ExecutionStrategies/Vanilla.cairo | 2 -- contracts/starknet/SpaceAccount.cairo | 1 + .../starknet/VotingStrategies/Whitelist.cairo | 7 +++---- contracts/starknet/lib/eip712.cairo | 17 +++++++++++++++++ contracts/starknet/lib/felt_utils.cairo | 7 ++++++- contracts/starknet/lib/session_key.cairo | 11 +++++++++++ contracts/starknet/lib/stark_eip191.cairo | 2 +- contracts/starknet/lib/uint256_utils.cairo | 14 ++++++++++++++ contracts/starknet/lib/voting.cairo | 8 +++++++- 9 files changed, 60 insertions(+), 9 deletions(-) create mode 100644 contracts/starknet/lib/uint256_utils.cairo diff --git a/contracts/starknet/ExecutionStrategies/Vanilla.cairo b/contracts/starknet/ExecutionStrategies/Vanilla.cairo index f0a3db8e..81f2f7b4 100644 --- a/contracts/starknet/ExecutionStrategies/Vanilla.cairo +++ b/contracts/starknet/ExecutionStrategies/Vanilla.cairo @@ -1,7 +1,5 @@ %lang starknet -from starkware.cairo.common.uint256 import Uint256 - @external func execute{syscall_ptr: felt*}( proposal_outcome: felt, execution_params_len: felt, execution_params: felt* diff --git a/contracts/starknet/SpaceAccount.cairo b/contracts/starknet/SpaceAccount.cairo index 533c68a2..c820e640 100644 --- a/contracts/starknet/SpaceAccount.cairo +++ b/contracts/starknet/SpaceAccount.cairo @@ -12,6 +12,7 @@ from contracts.starknet.lib.voting import Voting from contracts.starknet.lib.general_address import Address from contracts.starknet.lib.proposal_info import ProposalInfo from contracts.starknet.lib.vote import Vote +from contracts.starknet.lib.uint256_utils import Uint256Utils // // Constructor diff --git a/contracts/starknet/VotingStrategies/Whitelist.cairo b/contracts/starknet/VotingStrategies/Whitelist.cairo index 5a48cad0..f3d52e5d 100644 --- a/contracts/starknet/VotingStrategies/Whitelist.cairo +++ b/contracts/starknet/VotingStrategies/Whitelist.cairo @@ -1,8 +1,9 @@ %lang starknet -from starkware.cairo.common.uint256 import Uint256, uint256_check +from starkware.cairo.common.uint256 import Uint256 from starkware.cairo.common.cairo_builtins import HashBuiltin from contracts.starknet.lib.general_address import Address +from contracts.starknet.lib.uint256_utils import Uint256Utils @storage_var func whitelist(address: Address) -> (voting_power: Uint256) { @@ -45,9 +46,7 @@ func register_whitelist{syscall_ptr: felt*, pedersen_ptr: HashBuiltin*, range_ch // Add it to the whitelist let voting_power = Uint256(_whitelist[1], _whitelist[2]); - with_attr error_message("Whitelist: Invalid uint256 for voting power") { - uint256_check(voting_power); - } + Uint256Utils.assert_valid_uint256(voting_power); whitelist.write(address, voting_power); diff --git a/contracts/starknet/lib/eip712.cairo b/contracts/starknet/lib/eip712.cairo index 85ca434b..9bc721d5 100644 --- a/contracts/starknet/lib/eip712.cairo +++ b/contracts/starknet/lib/eip712.cairo @@ -21,6 +21,7 @@ from starkware.cairo.common.uint256 import ( ) from contracts.starknet.lib.felt_utils import FeltUtils from contracts.starknet.lib.array_utils import ArrayUtils +from contracts.starknet.lib.uint256_utils import Uint256Utils const ETHEREUM_PREFIX = 0x1901; @@ -68,6 +69,10 @@ namespace EIP712 { ) { alloc_locals; + Uint256Utils.assert_valid_uint256(r); + Uint256Utils.assert_valid_uint256(s); + Uint256Utils.assert_valid_uint256(salt); + let voter_address = calldata[0]; let (authenticator_address) = get_contract_address(); @@ -165,6 +170,10 @@ namespace EIP712 { ) { alloc_locals; + Uint256Utils.assert_valid_uint256(r); + Uint256Utils.assert_valid_uint256(s); + Uint256Utils.assert_valid_uint256(salt); + // Proposer address should be located in calldata[0] let proposer_address = calldata[0]; @@ -283,6 +292,10 @@ namespace EIP712 { ) -> () { alloc_locals; + Uint256Utils.assert_valid_uint256(r); + Uint256Utils.assert_valid_uint256(s); + Uint256Utils.assert_valid_uint256(salt); + // Ensure user has not already used this salt in a previous action let (already_used) = EIP712_salts.read(eth_address, salt); with_attr error_message("EIP712: Salt already used") { @@ -352,6 +365,10 @@ namespace EIP712 { ) -> () { alloc_locals; + Uint256Utils.assert_valid_uint256(r); + Uint256Utils.assert_valid_uint256(s); + Uint256Utils.assert_valid_uint256(salt); + // Ensure user has not already used this salt in a previous action let (already_used) = EIP712_salts.read(eth_address, salt); with_attr error_message("EIP712: Salt already used") { diff --git a/contracts/starknet/lib/felt_utils.cairo b/contracts/starknet/lib/felt_utils.cairo index fe078aa2..6752a390 100644 --- a/contracts/starknet/lib/felt_utils.cairo +++ b/contracts/starknet/lib/felt_utils.cairo @@ -2,6 +2,7 @@ from starkware.cairo.common.cairo_builtins import BitwiseBuiltin from starkware.cairo.common.math import unsigned_div_rem, split_felt, assert_nn_le from starkware.cairo.common.bitwise import bitwise_and from starkware.cairo.common.uint256 import Uint256 +from contracts.starknet.lib.uint256_utils import Uint256Utils const MAX_32 = 2 ** 32 - 1; @@ -22,7 +23,11 @@ namespace FeltUtils { ) { let word1_shifted = word1 * SHIFT_64; let word3_shifted = word3 * SHIFT_64; - return (Uint256(low=word3_shifted + word4, high=word1_shifted + word2),); + let result = Uint256(low=word3_shifted + word4, high=word1_shifted + word2); + + Uint256Utils.assert_valid_uint256(result); + + return (result,); } // Converts a felt to a Uint256. diff --git a/contracts/starknet/lib/session_key.cairo b/contracts/starknet/lib/session_key.cairo index 9946a94f..259abba6 100644 --- a/contracts/starknet/lib/session_key.cairo +++ b/contracts/starknet/lib/session_key.cairo @@ -10,6 +10,7 @@ from contracts.starknet.lib.stark_eip191 import StarkEIP191 from contracts.starknet.lib.eip712 import EIP712 from contracts.starknet.lib.eth_tx import EthTx from contracts.starknet.lib.array_utils import ArrayUtils +from contracts.starknet.lib.uint256_utils import Uint256Utils @storage_var func SessionKey_owner_store(session_public_key: felt) -> (eth_address: felt) { @@ -43,6 +44,11 @@ namespace SessionKey { session_duration: felt, ) { alloc_locals; + + Uint256Utils.assert_valid_uint256(r); + Uint256Utils.assert_valid_uint256(s); + Uint256Utils.assert_valid_uint256(salt); + EIP712.verify_session_key_init_sig( r, s, v, salt, eth_address, session_public_key, session_duration ); @@ -92,6 +98,11 @@ namespace SessionKey { ecdsa_ptr: SignatureBuiltin*, }(r: Uint256, s: Uint256, v: felt, salt: Uint256, session_public_key: felt) { alloc_locals; + + Uint256Utils.assert_valid_uint256(r); + Uint256Utils.assert_valid_uint256(s); + Uint256Utils.assert_valid_uint256(salt); + let (eth_address) = SessionKey_owner_store.read(session_public_key); with_attr error_message("SessionKey: Session does not exist") { assert_not_zero(eth_address); diff --git a/contracts/starknet/lib/stark_eip191.cairo b/contracts/starknet/lib/stark_eip191.cairo index d1cbc8b8..c2b4cad1 100644 --- a/contracts/starknet/lib/stark_eip191.cairo +++ b/contracts/starknet/lib/stark_eip191.cairo @@ -3,7 +3,7 @@ from starkware.starknet.common.syscalls import get_caller_address from starkware.cairo.common.cairo_builtins import HashBuiltin, SignatureBuiltin from starkware.cairo.common.math import assert_not_zero -from starkware.cairo.common.uint256 import uint256_eq, Uint256 +from starkware.cairo.common.uint256 import uint256_eq from starkware.cairo.common.alloc import alloc from starkware.cairo.common.signature import verify_ecdsa_signature from starkware.starknet.common.syscalls import get_contract_address diff --git a/contracts/starknet/lib/uint256_utils.cairo b/contracts/starknet/lib/uint256_utils.cairo new file mode 100644 index 00000000..239f882b --- /dev/null +++ b/contracts/starknet/lib/uint256_utils.cairo @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: MIT + +from starkware.cairo.common.uint256 import Uint256, uint256_check + +namespace Uint256Utils { + // Wrapper function to check if `uint256` is a valid `Uint256`. + // Wrapper is needed to have a proper error message. + func assert_valid_uint256{range_check_ptr}(uint256: Uint256) { + with_attr error_message("Uint256Utils: Invalid Uint256") { + uint256_check(uint256); + } + return (); + } +} diff --git a/contracts/starknet/lib/voting.cairo b/contracts/starknet/lib/voting.cairo index 181e36b3..b2866818 100644 --- a/contracts/starknet/lib/voting.cairo +++ b/contracts/starknet/lib/voting.cairo @@ -32,6 +32,7 @@ from contracts.starknet.lib.proposal_outcome import ProposalOutcome // Libraries from contracts.starknet.lib.array_utils import ArrayUtils, Immutable2DArray from contracts.starknet.lib.felt_utils import FeltUtils +from contracts.starknet.lib.uint256_utils import Uint256Utils // // Storage @@ -202,8 +203,9 @@ namespace Voting { assert_not_zero(voting_strategies_len); assert_not_zero(authenticators_len); assert_not_zero(executors_len); + Uint256Utils.assert_valid_uint256(proposal_threshold); + Uint256Utils.assert_valid_uint256(quorum); } - // TODO: maybe use uint256_signed_nn to check proposal_threshold? // Initialize the storage variables Voting_voting_delay_store.write(voting_delay); @@ -250,6 +252,8 @@ namespace Voting { ) { Ownable.assert_only_owner(); + Uint256Utils.assert_valid_uint256(new_quorum); + let (previous_quorum) = Voting_quorum_store.read(); Voting_quorum_store.write(new_quorum); @@ -319,6 +323,8 @@ namespace Voting { }(new_proposal_threshold: Uint256) { Ownable.assert_only_owner(); + Uint256Utils.assert_valid_uint256(new_proposal_threshold); + let (previous_proposal_threshold) = Voting_proposal_threshold_store.read(); Voting_proposal_threshold_store.write(new_proposal_threshold);