Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix account signature #387

Merged
merged 5 commits into from
Jul 12, 2022
Merged

Fix account signature #387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cd4df40
fix ecdsa_ptr
andrew-fleming Jul 9, 2022
bb84f27
remove unused vars
andrew-fleming Jul 9, 2022
4569949
bump nonce in test
andrew-fleming Jul 9, 2022
da12c6d
remove unused implicit arg
andrew-fleming Jul 9, 2022
918ce32
reference ecdsa_ptr issue
andrew-fleming Jul 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/Account.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,8 @@ Account contract developers are encouraged to implement the [standard Account in

To implement alternative `execute` functions, make sure to check their corresponding `validate` function before calling the `_unsafe_execute` building block, as each of the current presets is doing. Do not expose `_unsafe_execute` directly.

> Please note that the `ecdsa_ptr` implicit argument should be included in new methods that invoke `_unsafe_execute` (even if the `ecdsa_ptr` is not being used). Otherwise, it's possible that an account's functionalty can work in both the testing and local devnet environments; however, it could fail on public networks on account of the [SignatureBuiltinRunner](https://github.com/starkware-libs/cairo-lang/blob/master/src/starkware/cairo/lang/builtins/signature/signature_builtin_runner.py). See [issue #386](https://github.com/OpenZeppelin/cairo-contracts/issues/386) for more information.

Some other validation schemes to look out for in the future:

* multisig
Expand Down
3 changes: 2 additions & 1 deletion src/openzeppelin/account/EthAccount.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,14 +94,15 @@ func __execute__{
syscall_ptr : felt*,
pedersen_ptr : HashBuiltin*,
range_check_ptr,
ecdsa_ptr: SignatureBuiltin*,
bitwise_ptr: BitwiseBuiltin*
}(
call_array_len: felt,
call_array: AccountCallArray*,
calldata_len: felt,
calldata: felt*,
nonce: felt
) -> (response_len: felt, response: felt*):
) -> (response_len: felt, response: felt*):
let (response_len, response) = Account.eth_execute(
call_array_len,
call_array,
Expand Down
33 changes: 17 additions & 16 deletions src/openzeppelin/account/library.cairo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ namespace Account:
return (is_valid=TRUE)
end

func is_valid_eth_signature{
func is_valid_eth_signature{
syscall_ptr : felt*,
pedersen_ptr : HashBuiltin*,
bitwise_ptr: BitwiseBuiltin*,
Expand All @@ -169,7 +169,7 @@ namespace Account:
let sig_s : Uint256 = Uint256(low=signature[3], high=signature[4])
let (high, low) = split_felt(hash)
let msg_hash : Uint256 = Uint256(low=low, high=high)

let (local keccak_ptr : felt*) = alloc()

with keccak_ptr:
Expand All @@ -178,7 +178,7 @@ namespace Account:
r=sig_r,
s=sig_s,
v=sig_v,
eth_address=_public_key)
eth_address=_public_key)
end

return (is_valid=TRUE)
Expand All @@ -188,6 +188,7 @@ namespace Account:
syscall_ptr : felt*,
pedersen_ptr : HashBuiltin*,
range_check_ptr,
ecdsa_ptr: SignatureBuiltin*,
bitwise_ptr: BitwiseBuiltin*
}(
call_array_len: felt,
Expand All @@ -200,14 +201,12 @@ namespace Account:

let (__fp__, _) = get_fp_and_pc()
let (tx_info) = get_tx_info()
let (local ecdsa_ptr : SignatureBuiltin*) = alloc()
with ecdsa_ptr:
# validate transaction
with_attr error_message("Account: invalid signature"):
let (is_valid) = is_valid_signature(tx_info.transaction_hash, tx_info.signature_len, tx_info.signature)
assert is_valid = TRUE
end
end

# validate transaction
with_attr error_message("Account: invalid signature"):
let (is_valid) = is_valid_signature(tx_info.transaction_hash, tx_info.signature_len, tx_info.signature)
assert is_valid = TRUE
end

return _unsafe_execute(call_array_len, call_array, calldata_len, calldata, nonce)
end
Expand All @@ -216,6 +215,7 @@ namespace Account:
syscall_ptr : felt*,
pedersen_ptr : HashBuiltin*,
range_check_ptr,
ecdsa_ptr: SignatureBuiltin*,
bitwise_ptr: BitwiseBuiltin*
}(
call_array_len: felt,
Expand All @@ -229,29 +229,30 @@ namespace Account:
let (__fp__, _) = get_fp_and_pc()
let (tx_info) = get_tx_info()

# validate transaction
# validate transaction
with_attr error_message("Account: invalid secp256k1 signature"):
let (is_valid) = is_valid_eth_signature(tx_info.transaction_hash, tx_info.signature_len, tx_info.signature)
assert is_valid = TRUE
end

return _unsafe_execute(call_array_len, call_array, calldata_len, calldata, nonce)
end

func _unsafe_execute{
syscall_ptr : felt*,
pedersen_ptr : HashBuiltin*,
range_check_ptr,
ecdsa_ptr: SignatureBuiltin*,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

following Fran's comment, this shouldn't be needed right?

Copy link
Collaborator Author

@andrew-fleming andrew-fleming Jul 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frangio

Why is the ecdsa_ptr implicit argument required in unsafe_execute ?

Compilation without the ecdsa_ptr in _unsafe_execute fails:

E               starkware.cairo.lang.compiler.preprocessor.preprocessor_error.PreprocessorError: /Users/andrewfleming/Documents/starknet/cairo-contracts/cairo-contracts/.tox/default/lib/python3.7/site-packages/openzeppelin/account/library.cairo:211:9: Cannot convert the implicit arguments of _unsafe_execute to the implicit arguments of execute.
E                       return _unsafe_execute(call_array_len, call_array, calldata_len, calldata, nonce)
E                       ^*******************************************************************************^
E               The implicit arguments of '_unsafe_execute' were defined here:
E               /Users/andrewfleming/Documents/starknet/cairo-contracts/cairo-contracts/.tox/default/lib/python3.7/site-packages/openzeppelin/account/library.cairo:241:26
E                   func _unsafe_execute{
E                                        ^
E               The implicit arguments of 'execute' were defined here:
E               /Users/andrewfleming/Documents/starknet/cairo-contracts/cairo-contracts/.tox/default/lib/python3.7/site-packages/openzeppelin/account/library.cairo:187:18
E                   func execute{
E                                ^

_unsafe_execute makes the actual contract call and seems to require passing the sig builtin for verification:

https://github.com/starkware-libs/cairo-lang/blob/master/src/starkware/cairo/lang/builtins/signature/signature_builtin_runner.py#L12-L18

I'm still trying to understand this issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what if we explicitly pass the implicit arguments without including ecdsa_ptr? It looks like it's trying to pass along all the available implicit arguments including those that aren't needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand you question correctly, I don't think this works. Taking out the implicit ecdsa_ptr means we'll still need to create some reference to it to validate the signature...which brings us back to the same initial issue: Signature hint must point to the signature builtin segment, not {addr}.

Copy link
Contributor

@frangio frangio Jul 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion was to write the call as

_unsafe_execute{syscall_ptr, pedersen_ptr, etc...}(call_array_len, call_array, calldata_len, calldata, nonce)

(I'm not sure if this works)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Afaict the implicit arguments are not only received by _unsafe_execute but also passed to the next function call, doing so could revoke ecdsa_ptr but other than that it might work.

bitwise_ptr: BitwiseBuiltin*
}(
call_array_len: felt,
call_array: AccountCallArray*,
calldata_len: felt,
calldata: felt*,
calldata: felt*,
nonce: felt
) -> (response_len: felt, response: felt*):
alloc_locals

let (caller) = get_caller_address()
with_attr error_message("Account: no reentrant call"):
assert caller = 0
Expand All @@ -260,7 +261,7 @@ namespace Account:
# validate nonce

let (_current_nonce) = Account_current_nonce.read()

with_attr error_message("Account: nonce is invalid"):
assert _current_nonce = nonce
end
Expand Down
3 changes: 3 additions & 0 deletions tests/account/test_Account.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,9 @@ async def test_return_value(account_factory):
async def test_nonce(account_factory):
account, _, initializable, *_ = account_factory

# bump nonce
await signer.send_transactions(account, [(initializable.contract_address, 'initialized', [])])

Comment on lines +129 to +131
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this passing before? if so, why? if not, why is it needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was passing before, but with the proposed changes, the nonce's 0 - 1 is out of range for a valid signature, which in turn, returns a different error. This is similar to what happened here:
#361 (comment)

execution_info = await account.get_nonce().call()
current_nonce = execution_info.result.res

Expand Down
2 changes: 1 addition & 1 deletion tests/account/test_EthAccount.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ async def test_nonce(account_factory):
account, _, initializable, *_ = account_factory

# bump nonce
_, hash, signature = await signer.send_transactions(account, [(initializable.contract_address, 'initialized', [])])
await signer.send_transactions(account, [(initializable.contract_address, 'initialized', [])])

execution_info = await account.get_nonce().call()
current_nonce = execution_info.result.res
Expand Down