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

[Bug]: Ternary expression lifting breaks tuple assignments #2025

Closed
kevinclancy opened this issue Jul 3, 2023 · 3 comments · Fixed by #2120
Closed

[Bug]: Ternary expression lifting breaks tuple assignments #2025

kevinclancy opened this issue Jul 3, 2023 · 3 comments · Fixed by #2120
Labels
bug Something isn't working

Comments

@kevinclancy
Copy link
Contributor

kevinclancy commented Jul 3, 2023

Describe the issue:

The following source code:

contract Test {

    function getVal(uint z) internal returns(uint, uint) {
        return (0, 1);
    }

    function test(bool flag) external {
        uint a;
        (a, ) = getVal(flag ? 3 : 1);
    }
}

generates the following IR:

INFO:Printers:Contract Test
        Function Test.getVal(uint256) (*)
                Expression: (0,1)
                IRs:
                        RETURN 0,1
        Function Test.test(bool) (*)
                Expression: flag
                IRs:
                        CONDITION flag
                Expression: (a) = getVal(3)
                IRs:
                        TUPLE_0(uint256,uint256) = INTERNAL_CALL, Test.getVal(uint256)(3)
                        a(uint256) := TUPLE_0([<slither.core.solidity_types.elementary_type.ElementaryType object at 0x1023476d0>, <slither.core.solidity_types.elementary_type.ElementaryType object at 0x102347910>])
                Expression: (a) = getVal(1)
                IRs:
                        TUPLE_1(uint256,uint256) = INTERNAL_CALL, Test.getVal(uint256)(1)
                        a(uint256) := TUPLE_1([<slither.core.solidity_types.elementary_type.ElementaryType object at 0x1023476d0>, <slither.core.solidity_types.elementary_type.ElementaryType object at 0x102347910>])

The IR assigns the variable a of type uint256 to a tuple variable. Instead, a should store the projection (unpacking) of tuple element 0.

Code example to reproduce the issue:

contract Test {

function getVal(uint z) internal returns(uint, uint) {
    return (0, 1);
}

function test(bool flag) external {
    uint a;
    (a, ) = getVal(flag ? 3 : 1);
}

}

Version:

0.9.5

Relevant log output:

INFO:Printers:Contract Test
        Function Test.getVal(uint256) (*)
                Expression: (0,1)
                IRs:
                        RETURN 0,1
        Function Test.test(bool) (*)
                Expression: flag
                IRs:
                        CONDITION flag
                Expression: (a) = getVal(3)
                IRs:
                        TUPLE_0(uint256,uint256) = INTERNAL_CALL, Test.getVal(uint256)(3)
                        a(uint256) := TUPLE_0([<slither.core.solidity_types.elementary_type.ElementaryType object at 0x1023476d0>, <slither.core.solidity_types.elementary_type.ElementaryType object at 0x102347910>])
                Expression: (a) = getVal(1)
                IRs:
                        TUPLE_1(uint256,uint256) = INTERNAL_CALL, Test.getVal(uint256)(1)
                        a(uint256) := TUPLE_1([<slither.core.solidity_types.elementary_type.ElementaryType object at 0x1023476d0>, <slither.core.solidity_types.elementary_type.ElementaryType object at 0x102347910>])
@kevinclancy kevinclancy added the bug-candidate Bugs reports that are not yet confirmed label Jul 3, 2023
@kevinclancy
Copy link
Contributor Author

This can be fixed by modifying the ternary lifting code:

    def convert_expressions(
        self,
        expression: Union[AssignmentOperation, BinaryOperation, TupleExpression],
        true_expression: Expression,
        false_expression: Expression,
    ) -> None:
        for next_expr in expression.expressions:
            # TODO: can we get rid of `NoneType` expressions in `TupleExpression`?
            # montyly: this might happen with unnamed tuple (ex: (,,,) = f()), but it needs to be checked
            if next_expr is not None:

                if self.conditional_not_ahead(
                    next_expr, true_expression, false_expression, f_expressions
                ):
                    # always on last arguments added
                    self.copy_expression(
                        next_expr,
                        true_expression.expressions[-1],
                        false_expression.expressions[-1],
                    )
            else:
                true_expression.expressions.append(None)
                false_expression.expressions.append(None)

@0xalpharush 0xalpharush added bug Something isn't working and removed bug-candidate Bugs reports that are not yet confirmed labels Jul 6, 2023
@0xalpharush 0xalpharush changed the title [Bug-Candidate]: Ternary expression lifting breaks tuple assignments [Bug]: Ternary expression lifting breaks tuple assignments Jul 6, 2023
@0xalpharush
Copy link
Contributor

Would you submit a PR please?

@kevinclancy
Copy link
Contributor Author

Sure, I submitted a PR here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants