code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-07-swivel/blob/main/Swivel/Safe.sol#L41-L57

Vulnerability details

Impact

Potential memory pollution when calling transferFrom in Swivel/Safe.sol, which might have further consequences depending on usage of function.

Proof of Concept

In official SafeTransferLib.sol, it mstore arguments to freeMemoryPointer:

            mstore(add(freeMemoryPointer, 4), from) // Append the "from" argument.
            mstore(add(freeMemoryPointer, 36), to) // Append the "to" argument.
            mstore(add(freeMemoryPointer, 68), amount) // Append the "amount" argument.

But in Swivel/Safe.sol, it clobbers memory slot 0x80:

            mstore(0, 0x23b872dd00000000000000000000000000000000000000000000000000000000)
            mstore(4, from) // Append the "from" argument.
            mstore(36, to) // Append the "to" argument.
            mstore(68, amount) // Append the "amount" argument.

            success := and(
                // Set success to whether the call reverted, if not we check it either
                // returned exactly 1 (can't just be non-zero data), or had no return data.
                or(and(eq(mload(0), 1), gt(returndatasize(), 31)), iszero(returndatasize())),
                // We use 100 because that's the total length of our calldata (4 + 32 * 3)
                // Counterintuitively, this call() must be positioned after the or() in the
                // surrounding and() because and() evaluates its arguments from right to left.
                call(gas(), token, 0, 0, 100, 0, 32)
            )

            mstore(0x60, 0) // Restore the zero slot to zero.
            mstore(0x40, memPointer) // Restore the memPointer.

Tools Used

Manual Review

Recommended Mitigation Steps

Use SafeTransferLib.sol

JTraversa commented 2 years ago

I'm not 100% sure what to think of this one.

We've currently got the most up to date SafeTransfer implementation in the solmate v7 repo. We can revert to their older repo but would need further explanation.

robrobbins commented 2 years ago

no, freeMemoryPointer was being used in conjuction to add() to position the mstores 32 bytes apart starting, i'm presuming at 0x40 (64th byte).

the newer solmate just stores the first 4 bytes (function selector) at 0, and progresses 32-bytes at a time from there without any add s.

at the end 0x60 is zeroed out, and 0x40 is restored.

i don't think there's any pollution going on. If I'm wrong it's an issue for solmate and I can take it up there.

bghughes commented 2 years ago

Given there is not a clear attack vector outside of potential error and the scope of it being a dependency error - I believe this should be a Medium Issue.

JTraversa commented 2 years ago

I'd honestly hope for some actual verification before this is accepted as medium.

We are currently using the most up-to-date safeTransfer available, and rewarding a warden for reporting that as a bug wouldn't be a good idea unless they can give an example attack.

Just giving them a reward without any verification leaves us with no help really given, and no knowledge to bring to the Solmate developers, while rewarding potential laziness if its not actually valid

0xean commented 2 years ago

Warden needs to provide additional proof of concept for this issue to be accepted as M showing the pollution and the impact.

which might have further consequences depending on usage of function.

Downgrading to QA