openzeppelin/cairo-contracts

OpenZeppelin Contracts written in Cairo for Starknet, a decentralized ZK Rollup

cairo-contracts's Issues

ERC20 Bugs

Hey guys, I found a few bugs in the ERC20 contracts for Cairo:

• The transfer function uses get_caller_address which resolves to 0 if a user calls the contract directly
• No user authentication is taking place on any of the function calls

Is there somebody working on this? I'm happy to step in and contribute a pull request to finish this contract if it's needed. I'm trying to build some DEX contracts on top of this and can't progress too far without a standardized token contract.

Adding Hooks

Add Dev Hook Functions

Another feature request regarding the extensibility of Starknet smart contracts. Probably further down the list of things needed right now but good to be cognizant of while moving forward. The OpenZeppelin's Solidity ERC20 uses _beforeTokenTransfer and _afterTokenTransfer functions allowing functionality such as ERC20Pausable.

Upgrade to Cairo v0.5.0

• Remove StorageBuiltin and storage_ptr implicit args (they were merged into syscall_ptr)
• Add syscall_ptr implicit arg where necessary and use locals to prevent it from being revoked
• In Signer, update send_transaction to account for the new cairo-0.5.0 return values of call and invoke
• Adjust tests for the return values of call and invoke
• Replace initializers with constructors

Thanks to @RoboTeddy for providing a reference implementation in #39

Remove nonce from execute function's arguments

The current implementation of the Account contract requires the nonce as argument even though it's not used in the function. In fact the current nonce is read from a storage variable current_nonce, hence there's no need to have nonce as a function parameter.

Add Burn Function

Burn Function

• create _burn()
• apply viable means of testing
• include zero address (burner address) checks

Burn Function and Testing

Due to the uncertainty regarding how best to handle Starknet smart contract extensibility, the _burn() function sits in an awkward position. The function is declared as internal on OpenZeppelin's Solidity ERC20 base contract (ERC20.sol). In order to properly test the function's logic, an external wrapper function should be used to invoke _burn() from outside of the contract. The Solidity version created ERC20Mock.sol to test the base contract—their mock contract includes an external burn() function. For this repo, the feature should try and follow the Solidity implementation of ERC20. An approach to consider would be creating an external burn() function within contracts/ERC20.cairo. This, of course, assumes we don't find a practical solution for extensibility in the very near future.

Security Checks

The _burn() function should also include burner address protection as documented in issue #2 .

ERC20 Documentation

• API
• uint/felt compatibility
• example usage

Simplify ERC20 extension

Discussed in #103

^{Originally posted by martriay December 14, 2021}
I was thinking that maybe we could abstract ERC20 even further and move transferFrom, increaseAllowance, etc. implementations to the base contract, and require frontend contracts just to expose them instead of re-implementing them. What do you think?

Update to Cairo 0.6.0

https://medium.com/starkware/alpha-4-6b572072d80b

Add `name` and `symbol` to ERC20

Short string support should suffice.

Add ERC20 security checks

• add overflow checks
• protect burner address' coins

Overflow checks

The current ERC20 implementation does not implement any math overflow checks. One potential approach is to cap the the supply on the minting function, which would allow for any transfer to be automatically safe.

Burner address coins

Usually, the way to burn a token is to send it to an address that no one controls. In Ethereum, this is usually the Zero Address or 0x0. But in StarkNet, the get_caller_address() function will return 0 if there's no caller address. This can happen if a transaction is sent to a contract directly instead of through an account contract.

To accommodate to expected behavior, a check must be set in place to prevent someone to directly call the transfer() function on a token contract, impersonate the burner address, and break invariants.

ERC721 token

Add ERC721 documentation

Check uint256 overflows using the carry bit

AccessControlEnumerable

ERC20 `uint256` compatibility

To allow L1 bridged tokens to seamlessly integrate with this implementation, it should implement uint256 operations instead of felt, which takes up 252 bytes.

Memoize testing environment

ERC20 return bool on `transfer`

nile install fails - gmp.h file not found

Getting this error after 'nile install' when trying to follow the installation instructions on an m1 mac running Big Sur 11.5.2:

    creating build/temp.macosx-10.14.6-arm64-3.8/src
    clang -Wno-unused-result -Wsign-compare -Wunreachable-code -fno-common -dynamic -DNDEBUG -g -fwrapv -O3 -Wall -iwithsysroot/System/Library/Frameworks/System.framework/PrivateHeaders -iwithsysroot/Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.8/Headers -arch arm64 -arch x86_64 -Werror=implicit-function-declaration -Isrc/ -I/Users/cipherGarden/Documents/cipherGarden/tutorials/starkware/zeppelinCairo/cairo-contracts/env/include -I/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.8/include/python3.8 -c src/curveMath.c -o build/temp.macosx-10.14.6-arm64-3.8/src/curveMath.o -O2
    In file included from src/curveMath.c:1:
    src/curveMath.h:6:10: fatal error: 'gmp.h' file not found
    #include <gmp.h>
             ^~~~~~~
    1 error generated.
    error: command 'clang' failed with exit status 1
    ----------------------------------------

🔢 Code to reproduce bug

brew install gmp # mac
git clone https://github.com/OpenZeppelin/cairo-contracts.git
cd cairo-contracts
python3 -m venv env
source env/bin/activate
pip install cairo-nile
nile install

python version: 3.8.2

Add ERC721 Burnable preset

Add CI testing and set PR checks

Test failure when following the README

I have followed the README file and when I try to run tests with pytest I get the following error:

(env) user@users-MBP cairo-contracts % pytest
========================================================= test session starts =========================================================
platform darwin -- Python 3.8.7, pytest-6.2.5, py-1.10.0, pluggy-1.0.0
rootdir: /Users/user/dev/cairo-contracts
plugins: web3-5.24.0, hypothesis-6.24.0, eth-brownie-1.17.1, xdist-1.34.0, forked-1.3.0
collected 0 items / 5 errors                                                                                                          

=============================================================== ERRORS ================================================================
_______________________________________________ ERROR collecting tests/test_Account.py ________________________________________________
ImportError while importing test module '/Users/user/dev/cairo-contracts/tests/test_Account.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
../../.pyenv/versions/3.8.7/lib/python3.8/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
tests/test_Account.py:3: in <module>
    from starkware.starknet.testing.starknet import Starknet
E   ModuleNotFoundError: No module named 'starkware'

shouldn't nile install take care of the dependencies?

Account abstraction

Distribution mechanism

We need to think, decide, and implement a code distribution mechanism. Most probably a pypi package.

ERC721 Transfer token no token id

ERC721 contract transfer function is probably wrong

📝 Details

Transfer function of ERC721 contract should change not just the balance but the owner of the token as well.
What I see is just increase receiver balance and decrease sender balance like ERC20

🔢 Code to reproduce bug

func _transfer{
        syscall_ptr : felt*, 
        pedersen_ptr : HashBuiltin*,
        range_check_ptr
    }(sender: felt, recipient: felt, amount: felt):
    # validate sender has enough funds
    let (sender_balance) = balances.read(user=sender)
    assert_nn_le(amount, sender_balance)

    # substract from sender
    balances.write(sender, sender_balance - amount)

    # add to recipient
    let (res) = balances.read(user=recipient)
    balances.write(recipient, res + amount)
    return ()
end

Take a look, it's probably wrong.

Add `multicall` to Account

Check out relevant discussion: #27

Timelock

Crypto

No need to pass `this` to `execute`

There is no need to include the this parameter into every call to execute in L112 of Account.cairo and later check that it equals to the storage variable address in L150.

I believe it would be cleaner (less arguments to execute and less code overall) to simply read the value of address and include it in the Message struct in L120, i.e.

let (_address) = address.read()
local message: Message = Message(to, selector, calldata, calldata_size=calldata_len, _address, nonce) 

Add a view method to validate off-chain signatures

On L1 the verification of off-chain signatures is (unfortunately...) typically achieved via ecrecover. This only works for EOA wallets. EIP-1271 was developed to verify the off-chain signatures of smart-contract wallets.

With Account Abstraction we need a generic interface to validate off-chain signatures and all accounts should implement that interface, i.e. we need to make sure it becomes a standard for Accounts.

I would suggest something similar to EIP-1271:

const MAGIC_VALUE =  # get_selector_from_name('is_valid_signature')

@view
func is_valid_signature{
        storage_ptr: Storage*,
        pedersen_ptr: HashBuiltin*,
        ecdsa_ptr: SignatureBuiltin*,
        syscall_ptr: felt*,
        range_check_ptr
    } (
        hash: felt,
        signatures_len: felt,
        signatures: Signature*
    ) -> (magic_value: felt):
    # validate the signatures
    # return MAGIC_VALUE if the signatures are correct
end

where the method can receive an array of signatures to support EOA type accounts as well as multisig type accounts.

Add ERC721 Pausable preset

Transaction malleability in the account contract

Currently, the account contract does not verify that the nonce in the transaction call data is equal to the current_nonce in the contract, but only that the user signed on the current_nonce. This allows a malicious sequencer to replace nonce with an arbitrary felt. This results in a malleable vulnerability since the sequencer can change the transaction hash, while still being able to execute the transaction.

📝 Details

I suggest adding an assert in the execute() function, that verifies that the nonce in the call data is equal to the current_nonce in the contract storage.

Use Cairo's `hash_state` for message hashing

As stated by @lior-stark in #24, starkware.Cairo.common.hash_state does the same as Account's custom hash_message function but it's cheaper.

We should leverage that and reimplement Signer.py accordingly.

Module model

I am thinking there might be two ways to extend a contract, e.g., an ERC token to be Ownable.

1. A myToken.cairo with top-of-file import-style mechanism: from ozcc import ERC, Ownable.
2. A generator CLI program: ozcc generate ERC Ownable that produces a populated myToken.cairo.

Option 1 would mimic existing Solidity experience, and would involve functions overriding others in a hierarchy.

Option 2 would involve the program fetching only non-overridden functions from the chosen combination and placing those in a file.

Both options are compatible with pre-generated common patterns (721, 721-ownable, 721-ownable-enumerable, 721-enumerable). Option 2 might result in a less-complex contract file, because there would be no override patterns for the developer to mentally manage.

Redo README

• quickstart
• enumerate contracts
• run tests
• add nile?

eventually, when we figure it out:

• how to extend a contract

Update ERC20 interface to use `uint256`

Add documentation for utils

Reorganize contract directory structure

Base token contracts, their interfaces and presets are all thrown into the same folder today. We should think of a better way to organize files, with a focus on discoverability and ease of import paths declaration.

Upgrades proxy

Combining delegate_call and the upcoming fallback function.

Add Account base contract

Add basic multisig Account

This should be implemented in the is_valid_signature function.

This is how it looks for single signature accounts:

@view
func is_valid_signature{
        storage_ptr: Storage*,
        pedersen_ptr: HashBuiltin*,
        ecdsa_ptr: SignatureBuiltin*,
        syscall_ptr: felt*,
        range_check_ptr
    } (
        hash: felt,
        signature_len: felt,
        signature: felt*
    ) -> ():
    assert_initialized()
    let (_public_key) = public_key.read()
    # This interface expects a signature pointer and length to make
    # no assumption about signature validation schemes.
    # But this implementation does, and it expects a (sig_r, sig_s) pair.
    let sig_r = signature[0]
    let sig_s = signature[1]

    verify_ecdsa_signature(
        message=hash,
        public_key=_public_key,
        signature_r=sig_r,
        signature_s=sig_s)

    return ()
end

Uint256 Check for External Functions

Uint256 Check for External Functions

Overflow

Cairo's Uint256 library applies its own uint256_check() on all arithmetic functions; nevertheless, this does not account for uint256 arguments themselves. Each uint256 value is really a struct of two 128bit felts. Since Cairo's felt size equates to 252 bits, external function arguments easily overflow prior to reaching any security checks. Suppose a function, to further illustrate, accepts a uint256 argument, and a user inserts (2^128 + 1, 2^128). The values will be interpreted as (1, 0).

uint256_check

In order to ensure uint256 arguments (of two 128bit felts) do not exceed 2^256 - 1, a check should be implemented in all external functions accepting uint256 arguments as well as _mint() and _burn() (when added). uint256_check() should suffice from Cairo's Uint256.

• insert uint check in transfer()
• insert uint check in transfer_from()
• insert uint check in approve()
• insert uint check in increase_allowance()
• insert uint check in decrease_allowance()
• insert uint check in _mint()
• insert uint check in _burn()

Add increase/decrease allowance functions for ERC20

To prevent well known ERC20 approval issues, non-standard functions increaseAllowance and decreaseAllowance need to be implemented.

Update ERC20 Contract Nomenclature to Match Standard

Update ERC20 Contract Nomenclature to Match Standard

ERC20 Method Names

All snake_case functions should be changed to camelCase in order to match the standard put forth in EIP20. See the ERC20 function naming discussion.

Update ERC20 Parameters and Return Value Names

The parameters and return value names should mirror those set in EIP20. For example:

@view
func balance_of{
    ...
    }(account: felt) -> (res: Uint256):
    ...
end

res should be changed to balance.

AssertionError: normalize_address() cannot be used with the current constants.

Can't deploy simple ERC20 contract

No modification has been made to the contract
Quering the starknet network for the error I got:

filip@filip-pc:~$ starknet tx_status --hash 0x684194e5774d9c4b9d068cb84dbf9dc73ee19e0100528c9f09022044f760ca6 --network alpha-goerli --contract /path/to/my-project/starknet-artifacts/contracts/utils/token/ERC20.cairo/ERC20.json --error_message
/home/filip/.local/lib/python3.8/site-packages/starkware/starknet/common/storage.cairo:22:5: Error at pc=0:91:
    if is_small != 0:
    ^^
Got an exception while executing a hint.
Cairo traceback (most recent call last):
/path/to/my-project/contracts/utils/token/ERC20.cairo:19:6
func constructor{
     ^*********^
/path/to/my-project/contracts/utils/token/ERC20.cairo:29:5
    ERC20_initializer(name, symbol, initial_supply, recipient)
    ^********************************************************^
/path/to/my-project/contracts/utils/token/ERC20_base.cairo:54:5
    ERC20_mint(recipient, initial_supply)
    ^***********************************^
/path/to/my-project/contracts/utils/token/ERC20_base.cairo:135:30
    let (balance: Uint256) = ERC20_balances.read(account=recipient)
                             ^************************************^
autogen/starknet/storage_var/ERC20_balances/impl.cairo:16:30
        let (storage_addr) = addr(account)
                             ^***********^
autogen/starknet/storage_var/ERC20_balances/impl.cairo:10:21
        let (res) = normalize_address(addr=res)
                    ^*************************^

Traceback (most recent call last):
  File "<hint5>", line 5, in <module>
AssertionError: normalize_address() cannot be used with the current constants.

Add docs for extensibility pattern

Failed transactions can't be reprocessed due to current_nonce logic

Problem: Buggy signing code or a malicious actor can prevent an Account contract from being used by submitting a StarkNet transaction that fails due to invalid signature. Because StarkNet Alpha v4 does not allow resubmitting/updating a previously submitted transaction hash, a failed contract invocation due to invalid signature can guarantee future failures, even for future properly signed calls with the same transaction hash (same nonce + parameters).

Potential Solutions: StarkNet should allow reprocessing of same transaction hash or the Account contract should support nonces > current_nonce

Steps to reproduce:

1. Deploy Account contract
2. submit an execute transaction invoking a specific external contract, such as the Counter contract (https://github.com/fracek/starknet-react-example) with an increment of 1, but with an invalid signature
3. The transaction fails due to invalid signature
4. Reinvoke the same call with the same nonce + arguments, but include a valid signature
5. The transaction will not be reprocessed on StarkNet Alpha v4

Workaround: After the failed invocation with the bad signature, Invoke a different contract with a correct signature, which forces the nonce to increment, and you can make a successful invocation with the newly updated nonce

Note: It is unclear to me if the problem can occur with a correct signature. For example, if Alice makes an invocation that fails for some other reason, like an assertion failing due to a contract state. Then say the state on the target contract is updated by Bob, which should result in a success should Alice attempt to make the same call again. Alice's invocation could fail because StarkNet Alpha v4 will not reprocess the transaction due to having the same transaction hash.

Background: https://www.cairo-lang.org/docs/hello_starknet/user_auth.html#what-if-we-have-an-invalid-signature

Increase the Nonce in account contract

When I try sending execute method in Account contract, if the transaction is failed, the nonce is not update and I can't transfer again

• Problem
  I tried creating a transaction and sent it. It's failed then I can't call the transaction anymore, unless I call another transaction to increase the nonce, then come back again
• Solution
  So I think we can create an increase nonce method, to pass in the current nonce and update the nonce to the next nonce? So I can increase the nonce and call execute again.

Can't compile contract after fresh repo clone

I am trying to clone the repo and compile the contracts, and I follow the instructions.
An error pops up while I compile

contracts/Pausable_base.cairo:13:9: Unexpected implicit argument 'pedersen_ptr' in an external function.
        pedersen_ptr: HashBuiltin*,
        ^**********^
contracts/token/ERC20_base.cairo:65:9: Unexpected implicit argument 'pedersen_ptr' in an external function.
        pedersen_ptr : HashBuiltin*,
        ^**********^
contracts/token/ERC721.cairo:122:20: Unknown identifier 'total_supply'.
    let (supply) = total_supply.read()
                   ^***************^

The json artifacts are empty, and these files are not compiled.

📝 Details
Pretty much that's it
🔢 Code to reproduce bug

git clone https://github.com/OpenZeppelin/cairo-contracts
cd cairo-contracts
python3 -m venv env
source env/bin/activate
pip install cairo-nile
nile install
nile compile

Add Account documentation

• how execute works
• describe IAccount.cairo interface
• how are transactions structured
• API spec
• how to use Signer.py
• working example

String support

When discussing #9 we sort of ignored that implementing tokenUri requires a way to implement strings in Cairo. Strings are also used in ERC-20 (for the optional but so convenient symbol and name methods) and we have seen projects using them to generate dynamic NFT images (returning the NFT as svg).

In discord we mentioned that strings can be implemented by storing bytes in felts (251 bits or 31 bytes per felt). One issue is that, since StarkNet storage cannot store arrays, that's also the maximum string length that can be stored.