QA Report

[code423n4]

Summary

We found 1 low-critical finding and 1 non-critical finding:

• Low-Critical Findings - Uninitialized qTokenDetails.longStrikePrice for non-spread options in CollateralToken.sol/getCollateralTokenInfo
• Non-Critical Findings - Usage of ecrecover for metaTransactions without checking r/s in CollateralToken.sol/metaSetApprovalForAll

In summary of recommended security practices, it's better to initialize memory even if it's 0 and use public and verified libraries rather than craft by hand. The first finding is uninitialized memory, due to solidity doesn't guarantee uninitialized memory is almost always 0. It's better to initialize for improving security practices. The second finding is ecrecover. It's better to use a public and verified library like ECDSA. Because EIP-2 still allows signature malleability for ecrecover(), use ECDSA to remove this possibility and make the signature unique.

Low-Critical Findings - Uninitialized qTokenDetails.longStrikePrice for non-spread options in CollateralToken.sol/getCollateralTokenInfo

Impact

Uninitialize memory is almost always 0 due to solidity’s no memory recycle policy. However, this is not guaranteed in solidity documents and it would be better to initialize qTokenDetails.longStrikePrice. Although it won't cause any problems right now, it could be a potential threat in the future.

Proof of Concept

qTokensDetails.longStrikePrice isn't initialized when info.qTokenAsCollateral == address(0)

https://github.com/RollaProject/quant-protocol/blob/main/contracts/options/CollateralToken.sol#L237

    function getCollateralTokenInfo(uint256 id)
        external
        view
        override
        returns (QTokensDetails memory qTokensDetails)
    {
        CollateralTokenInfo memory info = idToInfo[id];

        require(
            info.qTokenAddress != address(0),
            "CollateralToken: Invalid id"
        );

        IQToken.QTokenInfo memory shortDetails = IQToken(info.qTokenAddress)
            .getQTokenInfo();

        qTokensDetails.underlyingAsset = shortDetails.underlyingAsset;
        qTokensDetails.strikeAsset = shortDetails.strikeAsset;
        qTokensDetails.oracle = shortDetails.oracle;
        qTokensDetails.shortStrikePrice = shortDetails.strikePrice;
        qTokensDetails.expiryTime = shortDetails.expiryTime;
        qTokensDetails.isCall = shortDetails.isCall;

        if (info.qTokenAsCollateral != address(0)) {
            // the given id is for a CollateralToken representing a spread
            qTokensDetails.longStrikePrice = IQToken(info.qTokenAsCollateral)
                .strikePrice();
        }
    }

Recommended Mitigation Steps

Initialize qTokensDetails.longStrikePrice

Non-Critical Findings - Usage of ecrecover for metaTransactions without checking r/s in CollateralToken.sol/metaSetApprovalForAll

Impact

Using ecrecover is against best practice. Preferably use ECDSA.recover instead. EIP-2 still allows signature malleability for ecrecover(). Remove this possibility and make the signature unique. However since address and nonce are included in message, it should be impossible to reuse or steal signatures.

Proof of Concept

https://github.com/RollaProject/quant-protocol/blob/main/contracts/options/CollateralToken.sol#L218

address signer = ecrecover(hash, v, r, s);

Recommended Mitigation Steps

Use ECDSA.recover instead

[alcueca]

Score: 27