code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-lukso/blob/9dbc96410b3052fc0fd9d423249d1fa42958cae8/contracts/LSP17ContractExtension/LSP17Extendable.sol#L86-L128

Vulnerability details

Summary

In _fallbackLSP17Extendable(), Calling functions that conditionally terminate the external EVM call using the assembly statements return(...) may result in incorrect removals of prior storage writes.

Impact

In LSP17Extendable.sol, _fallbackLSP17Extendable() is given as below. The contract is affected by the bug called Storage Write Removal Bug which was introduced in solidity version 0.8.13 and it has been fixed on Solidity version 0.8.17.

File: contracts/LSP17ContractExtension/LSP17Extendable.sol

86    function _fallbackLSP17Extendable() internal virtual {
        // If there is a function selector
        address extension = _getExtension(msg.sig);

        // if no extension was found, revert
        if (extension == address(0))
            revert NoExtensionFoundForFunctionSelector(msg.sig);

        // solhint-disable no-inline-assembly
        // if the extension was found, call the extension with the msg.data
        // appended with bytes20(address) and bytes32(msg.value)
        assembly {
            calldatacopy(0, 0, calldatasize())

            // The msg.sender address is shifted to the left by 12 bytes to remove the padding
            // Then the address without padding is stored right after the calldata
            mstore(calldatasize(), shl(96, caller()))

            // The msg.value is stored right after the calldata + msg.sender
            mstore(add(calldatasize(), 20), callvalue())

            // Add 52 bytes for the msg.sender and msg.value appended at the end of the calldata
            let success := call(
                gas(),
                extension,
                0,
                0,
                add(calldatasize(), 52),
                0,
                0
            )

            // Copy the returned data
            returndatacopy(0, 0, returndatasize())

            switch success
            // call returns 0 on failed calls
            case 0 {
                revert(0, returndatasize())
            }
            default {
127                return(0, returndatasize())
            }
        }
130    }

At L-127, the _fallbackLSP17Extendable() has used the inbuilt inline assembly return(...) in an inline assembly block which is the prerrequiste of this bug. Such an inline assembly call will not return from the current function, but instead result in an early successful (i.e. non-reverting) termination of the entire external EVM call.

Further bug description: A call to a Yul function that conditionally terminates the external EVM call could result in prior storage writes being incorrectly removed by the Yul optimizer. This used to happen in cases in which it would have been valid to remove the store, if the Yul function in question never actually terminated the external call, and the control flow always returned back to the caller instead. Conditional termination within the same Yul block instead of within a called function was not affected. In Solidity with optimized via-IR code generation, any storage write before a function conditionally calling return(...) or stop() in inline assembly, may have been incorrectly removed, whenever it would have been valid to remove the write without the return(...) or stop(). In optimized legacy code generation, only inline assembly that did not refer to any Solidity variables and that involved conditionally-terminating user-defined assembly functions could be affected.

Solidity official reference: https://soliditylang.org/blog/2022/09/08/storage-write-removal-before-conditional-termination/

Solidity has officially considered this bug as Medium/High severity finding which later fixed in version 0.8.17.