Some ERC20 tokens will not work with SafeTransferLib

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/libraries/SafeTransferLib.sol#L16

Vulnerability details

Impact

SafeTranferLib doesn’t support tokens like FRAX, UNI, CRV, COMP on its safeTransferFrom function, which will result in unexpected behavior for the system.

Proof of Concept

The SafeTransferLib used in this project is a simplified version of Solmate's SafeTransferLib. However, Solmate had to make some changes to address issues with the tokens mentioned above in the safeTransferFrom function. You can find more information about these issues in this GitHub issue: LinkThe fix for this problem can be found in this pull request: Link

Affected Code Example:

function safeTransferFrom(address token, address from, address to, uint256 amount) internal {
        bool success;

        assembly ("memory-safe") {
            // Get free memory pointer - we will store our calldata in scratch space starting at the offset specified here.
            let p := mload(0x40)

            // Write the abi-encoded calldata into memory, beginning with the function selector.
            mstore(p, 0x23b872dd00000000000000000000000000000000000000000000000000000000)
            mstore(add(4, p), from) // Append the "from" argument.
            mstore(add(36, p), to) // Append the "to" argument.
            mstore(add(68, p), 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, p, 100, 0, 32)
            )
        }

        if (!success) revert Errors.TransferFailed();
    }

Tools Used

manual review

Recommended Mitigation Steps

Use the latest version of the solmate library https://github.com/transmissions11/solmate/blob/4b47a19038b798b4a33d9749d25e570443520647/src/utils/SafeTransferLib.sol#L45C11-L49C1

Assessed type

DoS

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-sponsor]

dyedm1 (sponsor) disputed

[dyedm1]

dup #154

Ok the key point of the masking issue is if you put some garbage in the higher order bits of a 256-bit type and then cast it down, it stays on the stack until you cast it back to a higher type where the bits are actually relevant, so if you do that and then call this internal function the bits will get passed onto the call because the calldata is constructed using assembly. We, however, do not have this issue because the arguments are either msg.sender (guaranteed to be under 160bits) or the payer we decode from the callback which is always just going to be a real/actual user address. Unless you can find a case where the address that gets passed into safeTransfer actually has those garbage bits, the check would just be a waste of gas

[Picodes]

The sponsor's remark seems correct to me as there is no downcasting here, so in the absence of PoC I'll close as invalid.

[c4-judge]

Picodes marked the issue as unsatisfactory:
Invalid