TransferLib Manual Review Findings

TLB-01M: Incorrect Utilization of Memory

Type	Severity	Location
Language Specific		TransferLib.sol:L19-L20, L22-L26, L32, L46, L62, L65, L67-L69, L75, L90, L91

Description:

The TransferLib::transfer and TransferLib::transferFrom functions will insecurely handle memory space by writing to reserved slots as well as evaluating the free memory pointer without utilizing it.

Impact:

While the memory space is in a corrupted state after the TransferLib::transfer-prefixed functions have been invoked, the corruption should not manifest itself in practice unless affected statements are utilized, such as keccak256 assembly statements, usage of the free memory pointer, and/or instantiations of dynamic length arrays.

As this vulnerability would present itself and under certain conditions in upstream contracts, we cannot reliably assess its severity.

Example:

Copy
13/**
14 * @notice Transfer token amount.  Amount is sent from caller address to `to` address.
15 */
16function transfer(IERC20 token, address to, uint256 amount) internal {
17    bool success;
18    assembly ("memory-safe") {
19        // We'll write our calldata to this slot below, but restore it later.
20        let memPointer := mload(0x40)
21
22        // Write the abi-encoded calldata into memory, beginning with the function selector.
23        mstore(0, 0xa9059cbb00000000000000000000000000000000000000000000000000000000)
24        // Append arguments. Addresses are assumed clean. Transfer will fail otherwise.
25        mstore(0x4, to)
26        mstore(0x24, amount) // Append the "amount" argument.
27
28        // we have used up to 0x44 at this point
29
30        // fail if reverted; only allocate 32 bytes for return to ensure we
31        // only use mem slot 0
32        success := call(gas(), token, 0, 0, 68, 0, 32)
33        // handle transfers that return 1/true
34        let returnedOne := and(eq(mload(0), 1), gt(returndatasize(), 31))
35        // handle transfers that return nothing
36        let noReturn := iszero(returndatasize())
37        // good if didn't revert and the return is either empty or true
38        success := and(success, or(returnedOne, noReturn))
39
40        // to make the function call data, we used memory slots from:
41        // 0x0 - 0x44
42        //
43        // memory safety requires we consider:
44        // 0x0 - 0x40 is scratch space that we are free to use and leave in any state
45        // 0x40 - 0x60 is the mempointer which we restore here
46        mstore(0x40, memPointer) // Restore the memPointer.
47    }
48
49    unchecked {
50        if (!success) revert TransferFailed(token, to, amount);
51    }
52}

Recommendation:

We advise the code to make proper use of the free memory pointer updating it after the assembly block concludes, and to avoid utilizing the memory scratch space as well as preconfigured slots in the memory layout of Solidity.

Alleviation (175f8c39b1):

The code will now utilize the free memory pointer to store the ABI encoded call-data in memory, however, the data location of the free memory pointer is not properly updated once the function concludes.

For more details on how to properly maintain the free memory pointer, we advise the relevant Solidity documentation to be consulted.

Alleviation (23cf815e61):

After extensive discussion and research on the topic in collaboration with the Maverick Protocol team, we concluded that both the original and the latest approach are correct and in adherence to the memory model of Solidity.

In detail, the Solidity documentation outlines that memory beyond the free memory pointer can be utilized as scratch space freely and the original implementation properly restored the free memory pointer at the 0x40 data slot.

As the original exhibit was invalid and the latest implementation is secure, we consider this exhibit to be nullified.

TLB-02M: Inexistent Masking of Input Arguments

Type	Severity	Location
Language Specific		TransferLib.sol:L25, L67, L68

Description:

The referenced assembly statements will utilize address based input arguments that are not masked to erase their dirty bits. Per the Solidity documentation's security considerations section, data types that occupy less than 32 bytes may have dirty higher order bits that should be cleared when using assembly blocks.

Impact:

The present functions of the contract are not susceptible to such higher-order corruption due to properly storing a single argument per 32-byte slot, however, we still consider this a best practice recommendation to avoid potential compiler-level complications.

Example:

Copy
22// Write the abi-encoded calldata into memory, beginning with the function selector.
23mstore(0, 0xa9059cbb00000000000000000000000000000000000000000000000000000000)
24// Append arguments. Addresses are assumed clean. Transfer will fail otherwise.
25mstore(0x4, to)
26mstore(0x24, amount) // Append the "amount" argument.

Recommendation:

We advise a bit masking operation to be performed for each argument, ensuring that the address arguments do not contain any corrupt higher-order bits.

Alleviation (175f8c39b19df69134add3aa8a2a042ce3047763):

The Maverick Protocol team evaluated this exhibit and clarified that bit masking the input address values would incur unnecessary gas overhead as the relevant function calls would fail with incorrect arguments.

As such, we consider this exhibit safely acknowledged.