Memory alias causes linked structs to manipulate eachother

[Alexangelj]

Ref: https://samczsun.com/paradigm-ctf-2021-swap/

Problem

Coincidentally, our swap method is structured very similarly to this CTF problem: https://github.com/primitivefinance/portfolio/blob/285279ae38accfc7fe2f25ec67600dd7c6d9345d/contracts/Portfolio.sol#L403

Since it takes the argument in memory, it enables an attacker to weaponize the size of memory to force the free memory pointer to overflow, and thus re-allocate in use memory... that's scary.

Solution

Storing the swap arguments in calldata instead of memory should resolve this issue. I added a test test_swap_returns_native_decimals, which uncovered the args.output was returning a WAD scaled value, even though it was never altered in the code: https://github.com/primitivefinance/portfolio/pull/422/commits/b1d98da0a9ced77f6c4cc0fdac77c3783bbcf4ce

Replicate issue

Clone that commit https://github.com/primitivefinance/portfolio/pull/422/commits/b1d98da0a9ced77f6c4cc0fdac77c3783bbcf4ce, then run:

forge test -vvv --match-test test_swap_returns_native_decimals

Notice how the output is in WAD units, but it's never changed in the swap() function?

Note

This makes me concerned about other memory allocation issues, especially since we heavily use structs and memory across the codebase, along with the via-ir pipeline.

[MrToph]

The issue is here:

function swap(Order memory args) {

    Order memory orderInWad = args; // points to existing args
    orderInWad.input = inter.amountInputUnit.safeCastTo128(); // writes to args

    return args.input;
}

Order memory orderInWad = args is just a memory pointer to the existing args struct in memory, it does NOT create a copy of *args and point orderInWad to it. So whenever you write to orderInWad, you're also changing args and therefore the return values args.input/output are changed.

orderInWad.input = inter.amountInputUnit which is a WAD value is also written to args because it's just an alias. Changing the argument args to calldata fixes this because then this assignment to memory actually does a copy (because you can't have a memory pointer to calldata, you must copy the calldata to memory first to point to it).