[APY-02M][APY-03M] Potentially Insecure Utilization of Scratch Space

[pde-rent]

Recommendation:

We strongly advise against utilizing the scratch space based on the fact that the AsProxy::_delegateWithSignature function is invoked within other functions, the usage of keccak256 prior to the assembly block which utilizes the scratch space itself, and the fact that the memory required by the function is dynamic and reliant on the call-data of the top-level call.

Good point, corrupting memory and messing with the scratch space is not suitable for use at scale. We'll change the implementation to not alter scratch space and keep track of the updated memory pointer to make _delegateWithSignature usable amidst a longer call-flow.

I would argue the severity here could be medium given that the function works (used for 1 year+) and stack corruption is hardly productible, the issues arise from broader call-flows (eg. designated inputs missmatch) and not the delegation itself, what do you think?

Fix proposal

function _delegateWithSignature(
    address _implementation,
    string memory _signature
) internal {
    assembly {
        let selector := keccak256(add(_signature, 0x20), mload(_signature)) // hash fn signature
        selector := and(selector, 0xffffffff) // extract the selector (4 1st bytes)
        let calldataSize := add(calldatasize(), 0x4) // alloc calldata memory: selector + original calldata
        let callData := mload(0x40) // free memory ptr
        mstore(callData, selector) // store selector at the beginning of the new calldata
        calldatacopy(add(callData, 0x4), 0x4, sub(calldatasize(), 0x4)) // copy the rest
        mstore(0x40, add(callData, calldataSize)) // update free memory ptr

        let result := delegatecall(
            gas(),
            _implementation,
            callData,
            calldataSize,
            0,
            0
        )
        let size := returndatasize()
        let ptr := mload(0x40) // load free memory ptr for return data
        returndatacopy(ptr, 0, size)
        mstore(0x40, add(ptr, size)) // update free memory ptr

        switch result
        case 0 {
            revert(ptr, size)
        }
        default {
            return(ptr, size)
        }
    }
}

Edit Here is an updated fix proposal with delegate calldata being passed as parameter

function _delegateWithSignature(
    address _implementation,
    string calldata _signature,
    bytes calldata params
) internal {
    assembly {
        let selector := keccak256(add(_signature, 0x20), mload(_signature))
        selector := and(selector, 0xffffffff)
        let paramsSize := calldataload(params.offset)
        let calldataSize := add(0x4, paramsSize)
        let callData := mload(0x40)
        mstore(callData, selector)
        calldatacopy(add(callData, 0x4), params.offset, paramsSize) // copy params after the selector in the new calldata
        mstore(0x40, add(callData, calldataSize))
        let result := delegatecall(
            gas(),
            _implementation,
            callData,
            calldataSize,
            0,
            0
        )
        let size := returndatasize()
        let ptr := mload(0x40)
        returndatacopy(ptr, 0, size)
        mstore(0x40, add(ptr, size))
        switch result
        case 0 {
            revert(ptr, size)
        }
        default {
            return(ptr, size)
        }
    }
}

Edit 2 Function selector should be passed to the function from interface pre-compiled and not re-compiled at RT

[omniscia-core]

I believe there is slight confusion in relation to this issue. The issue title includes APY-02M which is about scratch space and APY-03M which is about an improperly forwarded payload.

APY-02M is already minor (due to the difficulties in reproducing stack corruption as you described) but APY-03M is reproducible and directly outlines the flaw in the forwarded calldata which we reproduced locally. We are more than happy to relocate the major severity vulnerability to the StrategyV5Chainlink::updateAsset and StrategyV5Pyth::updateAsset functions, however, this would result in two major vulnerabilities replacing one which we do not think is ideal.

[pde-rent]

Thanks for clarifying, let's keep it as is. We implemented the fix in a19c9dfd5403356cd0fb0ab701067c237a8696c5, tests revealed the implementation had flaws leading to 01dec1f31b6a983353be1fbb72aeb8163bdac34d. Please note that _delegateToSelectorMemory still has a an issue that we're working on (used in updateAsset and setInputs) if you guys could have a look, it might be obvious to your eagle eyes