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.

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

Passing arrays to private and public functions #1418

Closed
robinsierra opened this issue May 6, 2019 · 9 comments · Fixed by #1429
Labels
bug

Comments

@robinsierra
Copy link

@robinsierra robinsierra commented May 6, 2019

  • vyper Version: 0.1.0b9
  • Tested with Remix IDE

I'm trying to understand how arrays work in Vyper. It seems that they always get copied on assignment. However, I found the following:

@public
def change_arr(arr: int128[5]):
    a: int128[5] = arr
    a[0] = 24

@public
def call_arr() -> int128:
    a: int128[5]
    a[0] = 42
    self.change_arr(a)
    return a[0]

As expected, calling the function call_arr returns 42. However, if I make the function change_arr private the execution ends in an exception (because of a[0] = 24).

@charles-cooper

This comment has been minimized.

Copy link
Collaborator

@charles-cooper charles-cooper commented May 6, 2019

What version of vyper are you on?

@robinsierra

This comment has been minimized.

Copy link
Author

@robinsierra robinsierra commented May 6, 2019

I tested it with the Vyper plugin in the Remix IDE which seems to be using version 0.1.0b9.

@charles-cooper

This comment has been minimized.

Copy link
Collaborator

@charles-cooper charles-cooper commented May 7, 2019

Thanks - you mentioned your execution ends in an exception, what specific exception are you seeing?

@robinsierra

This comment has been minimized.

Copy link
Author

@robinsierra robinsierra commented May 7, 2019

On closer inspection I found the following: On instruction no 522 (which is an MSTORE) there is only a single value on the operand stack, instead of the required 2.

@charles-cooper

This comment has been minimized.

Copy link
Collaborator

@charles-cooper charles-cooper commented May 8, 2019

This seems like a bug. Trying to reproduce it on my end, in the meantime would it be possible for you to post the code which produces this? Or the IR (vyper -f ir) with some context surrounding the MSTORE in question - I just want to be able to correlate the source code with the IR.

@robinsierra

This comment has been minimized.

Copy link
Author

@robinsierra robinsierra commented May 8, 2019

The code above (with private instead of public) is all that is needed:

@private
def change_arr(arr: int128[5]):
    a: int128[5] = arr
    a[0] = 24

@public
def call_arr() -> int128:
    a: int128[5]
    a[0] = 42
    self.change_arr(a)
    return a[0]

Calling call_arr() triggers the bug. I think the bottom most [mstore, 416, pass] right at the end of line 10 is the one that makes it fail.

IR:

[seq,
  [return,
    0,
    [lll,
      [seq,
        [mstore, 28, [calldataload, 0]],
        [mstore, 32, 1461501637330902918203684832716283019655932542976],
        [mstore, 64, 170141183460469231731687303715884105727],
        [mstore, 96, -170141183460469231731687303715884105728],
        [mstore, 128, 1701411834604692317316873037158841057270000000000],
        [mstore, 160, -1701411834604692317316873037158841057280000000000],
        # Line 1
        [if,
          0,
          [seq,
            /* change_arr(int128[5]) */ [label, priv_3996084614],
            /* pop callback pointer */ [mstore, 480, pass],
            [mstore, 320, pass],
            [mstore, 352, pass],
            [mstore, 384, pass],
            [mstore, 416, pass],
            [mstore, 448, pass],
            [with,
              _L,
              512,
              [with,
                _R,
                '320' <arr>,
                [seq,
                  [mstore,
                    [add, [mul, 32, [uclamplt, 0, 5]], _L],
                    [mload, [add, [mul, 32, [uclamplt, 0, 5]], _R]]],
                  [mstore,
                    [add, [mul, 32, [uclamplt, 1, 5]], _L],
                    [mload, [add, [mul, 32, [uclamplt, 1, 5]], _R]]],
                  [mstore,
                    [add, [mul, 32, [uclamplt, 2, 5]], _L],
                    [mload, [add, [mul, 32, [uclamplt, 2, 5]], _R]]],
                  [mstore,
                    [add, [mul, 32, [uclamplt, 3, 5]], _L],
                    [mload, [add, [mul, 32, [uclamplt, 3, 5]], _R]]],
                  [mstore,
                    [add, [mul, 32, [uclamplt, 4, 5]], _L],
                    [mload, [add, [mul, 32, [uclamplt, 4, 5]], _R]]]]]],
            # Line 4
            [mstore, [add, [mul, 32, [uclamplt, 0, 5]], '512' <a>], 24],
            # Line 1
            [jump, [mload, 480]]]],
        # Line 6
        [if,
          [eq, [mload, 0], '3032955471' <call_arr()>],
          [seq,
            [assert, [iszero, callvalue]],
            # Line 9
            [mstore, [add, [mul, 32, [uclamplt, 0, 5]], '320' <a>], 42],
            # Line 10
            /* Internal Call: change_arr */ 
            [pop,
              [seq_unchecked,
                [mload, 320],
                [mload, 352],
                [mload, 384],
                [mload, 416],
                [mload, 448],
                [seq,
                  [mstore, 480, 3996084614],
                  [with,
                    _L,
                    512,
                    [with,
                      _R,
                      '320' <a>,
                      [seq,
                        [mstore,
                          [add, [mul, 32, [uclamplt, 0, 5]], _L],
                          [mload, [add, [mul, 32, [uclamplt, 0, 5]], _R]]],
                        [mstore,
                          [add, [mul, 32, [uclamplt, 1, 5]], _L],
                          [mload, [add, [mul, 32, [uclamplt, 1, 5]], _R]]],
                        [mstore,
                          [add, [mul, 32, [uclamplt, 2, 5]], _L],
                          [mload, [add, [mul, 32, [uclamplt, 2, 5]], _R]]],
                        [mstore,
                          [add, [mul, 32, [uclamplt, 3, 5]], _L],
                          [mload, [add, [mul, 32, [uclamplt, 3, 5]], _R]]],
                        [mstore,
                          [add, [mul, 32, [uclamplt, 4, 5]], _L],
                          [mload, [add, [mul, 32, [uclamplt, 4, 5]], _R]]]]]]],
                [mstore, 736, 672],
                [label, push_args_3996084614_10_4_start],
                [if, [lt, [mload, 736], 544], [goto, push_args_3996084614_10_4_end]],
                [if_unchecked, [ne, [mload, [mload, 736]], 0], [mload, [mload, 736]]],
                [mstore, 736, [sub, [mload, 736], 32]],
                [goto, push_args_3996084614_10_4_start],
                [label, push_args_3996084614_10_4_end],
                [mload, 512],
                [add, pc, 6],
                [goto, priv_3996084614],
                jumpdest,
                [mstore, 448, pass],
                [mstore, 416, pass],                                                                  
                [mstore, 384, pass],
                [mstore, 352, pass],
                [mstore, 320, pass],
                0]],
            # Line 11
            [mstore, 0, [mload, [add, [mul, 32, [uclamplt, 0, 5]], '320' <a>]]],
            [return, 0, 32],
            # Line 6
            stop]],
        /* Default function */ [revert, 0, 0]],
      0]]]
@charles-cooper

This comment has been minimized.

Copy link
Collaborator

@charles-cooper charles-cooper commented May 8, 2019

This seems to be a bug in argument packing (the inner assignment is not needed to trip the issue). The following is a minimal difference between the non-error and error cases

@private
def change_arr(arr: int128[2]):
    pass
@public
def call_arr() -> int128:
-    a: int128[2] = [42,1] # pass
+    a: int128[2]          # fail
    self.change_arr(a)
    return 42
@charles-cooper

This comment has been minimized.

Copy link
Collaborator

@charles-cooper charles-cooper commented May 9, 2019

An even more subtle test case:

@private
def change_arr(arr: int128[2]):
    pass
@public
def call_arr() -> int128:
    a: int128[2]
+    a[0] = 37 # fail
-    a[1] = 37 # pass
    self.change_arr(a)
    return 42

Difference in IR:

<             [mstore, [add, [mul, 32, [uclamplt, 1, 2]], '320' <a>], 37],
---
>             [mstore, [add, [mul, 32, [uclamplt, 0, 2]], '320' <a>], 37],
@fubuloubu

This comment has been minimized.

Copy link
Member

@fubuloubu fubuloubu commented May 28, 2019

Security Alert: if you use arrays as parameters to a private function, on released versions starting at v0.1.0-beta.4 (introduced by #1012) it could possibly cause a 'out of stack items' reversion. This could be used in a Denial of Service attack, under certain scenarios where the attacker is able to manipulate the stack items. More severe versions of this vulnerability can be used by an attacker to access arbitrary memory if the right conditions make that possible.

You should upgrade to v0.1.0-beta.10 to fix this issue.

fubuloubu added a commit that referenced this issue May 28, 2019
fubuloubu added a commit that referenced this issue May 28, 2019
Add Disclosure for #1418
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.