Uint256 Implementation Check

[andrew-fleming]

Uint256 Implementation Check

Preface

This PR, in reference to #52 , includes test cases that attempt to break the ERC20 implementation (at the time of this writing). It should be understood that inserting overflowing arguments in functions such as approve() and increase_allowance() will not return an error because of said overflow. Prior to reaching the uint256 arithmetic functions, which include a check on the uint256 values themselves, these values simply overflow. For example: if you attempt to call approve() with the arguments 2**128 + 2, 0, the transaction will not revert because the value is interpreted as 1, 0. This does not break the ERC20 implementation because the integers overflow prior to reaching the ERC20 state.

Summary

This PR includes three tests that check for potential vulnerabilities regarding overflow in respect to ERC20 functions accepting uint256 arguments from Cairo's Uint256 library. decrease_allowance() proves to be the most straight-forward function to check. The function is displayed below for reference.

@external
func decrease_allowance{
        syscall_ptr : felt*, 
        pedersen_ptr : HashBuiltin*,
        range_check_ptr
    }(spender: felt, subtracted_value: Uint256):
    alloc_locals
    let (local caller) = get_caller_address()
    let (local current_allowance: Uint256) = allowances.read(owner=caller, spender=spender)
    let (local new_allowance: Uint256) = uint256_sub(current_allowance, subtracted_value)

    # validates new_allowance < current_allowance and returns 1 if true   
    let (enough_allowance) = uint256_lt(new_allowance, current_allowance)
    assert_not_zero(enough_allowance)

    _approve(caller, spender, new_allowance)
    return()
end

Notice, the enough_allowance check occurs after current_allownace - subtracted_value. If the ERC20 implementation can be corrupted, this particular spot is the most vulnerable. The test first approves MAX_AMOUNT (2**256 - 1), and then proceeds to test whether our implementation will behave in an unsuspecting manner via single felt overflows. This test fails each iteration of overflowing parameters.

Further, this PR also includes the same type of test for transfer() and transfer_from(). Being that their respective function logic has a few more moving parts than decrease_allowance(), it seemed apropos to include them. Perhaps, testing decrease_allowance() is enough for the sake of brevity.

Implementation

• Approved the maximum integer to prove that any number within that range would not fail the test
• Tested the functions with the following arguments:
  -- 2^128 - 1, 2^128
  -- 2^128, 2^128 - 1
  -- 0, 2^128 + 2
  -- 2^128 + 2, 0

[martriay]

In the case of _transfer (therefore transfer and transferFrom), no checks are needed thanks to the enough_balance check. As long as sender_balance is a valid Uint256, uint256_le is enough because it guarantees that amount's high and low bits are not larger felts than sender_balance's.

func _transfer{
        syscall_ptr : felt*, 
        pedersen_ptr : HashBuiltin*,
        range_check_ptr
    }(sender: felt, recipient: felt, amount: Uint256):
    alloc_locals
    assert_not_zero(sender)
    assert_not_zero(recipient)

    let (local sender_balance: Uint256) = balances.read(account=sender)

    # validates amount <= sender_balance and returns 1 if true
    let (enough_balance) = uint256_le(amount, sender_balance)
    assert_not_zero(enough_balance)

We need to make sure mint(), approve(), and increase_allowance() are safe too. I'm honestly not sure about the safety of mint() since uint256_add assumes valid Uint256 inputs and we're not sanitizing first; so I wonder if passing a corrupt uint256 large enough to overflow a felt couldn't result in a smaller new_balance.

func _mint{
        syscall_ptr : felt*, 
        pedersen_ptr : HashBuiltin*,
        range_check_ptr
    }(recipient: felt, amount: Uint256):
    alloc_locals
    assert_not_zero(recipient)

    let (balance: Uint256) = balances.read(account=recipient)
    # overflow is not possible because sum is guaranteed to be less than total supply
    # which we check for overflow below
    let (new_balance, _: Uint256) = uint256_add(balance, amount)

Considering that StarkNet goes live on mainnet soon, I'm leaning towards just adding the checks and only removing them once we have concluded the research.