slither icon indicating copy to clipboard operation
slither copied to clipboard

Published 20 hours ago • verified publisher icon crytic

[Bug-Candidate]: slither-check-upgradeability different varible false postive

Open zowie951 opened this issue 3 years ago • 2 comments

Describe the issue:

slither-check-upgradeability false positive with the immutable varible.

Code example to reproduce the issue:

/** *Submitted for verification at Etherscan.io on 2022-05-25 */

// SPDX-License-Identifier: LGPL-3.0-or-later // Taken from: https://github.com/gnosis/safe-contracts/blob/development/contracts/proxies/GnosisSafeProxy.sol pragma solidity ^0.7.0;

/// @title IProxy - Helper interface to access masterCopy of the Proxy on-chain /// @author Richard Meissner - [email protected] interface IProxy { function masterCopy() external view returns (address); }

/// @title WalletProxy - Generic proxy contract allows to execute all transactions applying the code of a master contract. /// @author Stefan George - [email protected] /// @author Richard Meissner - [email protected] contract WalletProxy {

// masterCopy always needs to be first declared variable, to ensure that it is at the same location in the contracts to which calls are delegated.
// To reduce deployment costs this variable is internal and needs to be retrieved via `getStorageAt`
address internal masterCopy;

/// @dev Constructor function sets address of master copy contract.
/// @param _masterCopy Master copy address.
constructor(address _masterCopy)
{
    require(_masterCopy != address(0), "Invalid master copy address provided");
    masterCopy = _masterCopy;
}

/// @dev Fallback function forwards all transactions and returns all received return data.
fallback()
payable
external
{
    // solium-disable-next-line security/no-inline-assembly
    assembly {
        let _masterCopy := and(sload(0), 0xffffffffffffffffffffffffffffffffffffffff)
    // 0xa619486e == keccak("masterCopy()"). The value is right padded to 32-bytes with 0s
        if eq(calldataload(0), 0xa619486e00000000000000000000000000000000000000000000000000000000) {
            mstore(0, _masterCopy)
            return(0, 0x20)
        }
        calldatacopy(0, 0, calldatasize())
        let success := delegatecall(gas(), _masterCopy, 0, calldatasize(), 0, 0)
        returndatacopy(0, 0, returndatasize())
        if eq(success, 0) { revert(0, returndatasize()) }
        return(0, returndatasize())
    }
}

}

contract SmartWallet is ILoopringWalletV2, ERC1271, IERC165, ERC721Holder, ERC1155Holder { using ERC20Lib for Wallet; using ERC1271Lib for Wallet; using LockLib for Wallet; using GuardianLib for Wallet; using InheritanceLib for Wallet; using MetaTxLib for Wallet; using WhitelistLib for Wallet; using QuotaLib for Wallet; using RecoverLib for Wallet; using UpgradeLib for Wallet;

bytes32     public immutable DOMAIN_SEPARATOR;
PriceOracle public immutable priceOracle;
address     public immutable blankOwner;

// WARNING: Do not delete wallet state data to make this implementation
// compatible with early versions.
//
//  ----- DATA LAYOUT BEGINS -----
// Always needs to be first
address internal masterCopy;

bool internal isImplementationContract;

Wallet public wallet;
//  ----- DATA LAYOUT ENDS -----

/// @dev We need to make sure the implemenation contract cannot be initialized
///      and used to do delegate calls to arbitrary contracts.
modifier disableInImplementationContract
{
    require(!isImplementationContract, "DISALLOWED_ON_IMPLEMENTATION_CONTRACT");
    _;
}

modifier onlyFromWalletOrOwnerWhenUnlocked()
{
    // If the wallet's signature verfication passes, the wallet must be unlocked.
    require(
        msg.sender == address(this) ||
        (msg.sender == wallet.owner && !wallet.locked),
         "NOT_FROM_WALLET_OR_OWNER_OR_WALLET_LOCKED"
    );
    wallet.touchLastActiveWhenRequired();
    _;
}

modifier canTransferOwnership()
{
    require(
        msg.sender == blankOwner &&
        wallet.owner == blankOwner,
        "NOT_ALLOWED_TO_SET_OWNER"
    );
    _;
}

constructor(
    PriceOracle _priceOracle,
    address     _blankOwner
    )
{
    isImplementationContract = true;

    DOMAIN_SEPARATOR = EIP712.hash(
        EIP712.Domain("LoopringWallet", "2.0.0", address(this))
    );

    priceOracle = _priceOracle;
    blankOwner = _blankOwner;
}

/// @dev Set up this wallet.
///
///      Note that calling this method more than once will throw.
///
/// @param owner The owner of this wallet, must not be address(0).
/// @param guardians The guardians of this wallet.
function initialize(
    address             owner,
    address[] calldata  guardians,
    uint                quota,
    address             inheritor,
    address             feeRecipient,
    address             feeToken,
    uint                feeAmount
    )
    external
    override
    disableInImplementationContract
{
    require(wallet.owner == address(0), "INITIALIZED_ALREADY");
    require(owner != address(0), "INVALID_OWNER");

    wallet.owner = owner;
    wallet.creationTimestamp = uint64(block.timestamp);
    wallet.addGuardiansImmediately(guardians);

    if (quota != 0) {
        wallet.setQuota(quota, 0);
    }

    if (inheritor != address(0)) {
        wallet.setInheritor(inheritor, 365 days);
    }

    // Pay for the wallet creation using wallet funds
    if (feeRecipient != address(0) && feeAmount > 0) {
        ERC20Lib.transfer(feeToken, feeRecipient, feeAmount);
    }
}

receive()
    external
    payable
{
}

function getOwner()
    public
    view
    override
    returns (address)
{
    return wallet.owner;
}

function getCreationTimestamp()
    public
    view
    override
    returns (uint64)
{
    return wallet.creationTimestamp;
}

//
// Owner
//
function transferOwnership(
    address _owner
    )
    external
    canTransferOwnership
{
    require(_owner != address(0), "INVALID_OWNER");
    wallet.owner = _owner;
}

//
// ERC1271
//
function isValidSignature(
    bytes32      signHash,
    bytes memory signature
    )
    public
    view
    override
    returns (bytes4 magicValue)
{
    return wallet.isValidSignature(
        ERC1271_MAGICVALUE,
        signHash,
        signature
    );
}

//
// Upgrade
//

function changeMasterCopy(
    Approval calldata approval,
    address           newMasterCopy
    )
    external
    returns (bytes32 approvedHash)
{
    approvedHash = wallet.changeMasterCopy(
        DOMAIN_SEPARATOR,
        approval,
        newMasterCopy
    );
    masterCopy = newMasterCopy;
}

function getMasterCopy()
    public
    view
    returns (address)
{
    return masterCopy;
}

//
// Guardians
//

function addGuardian(
    address guardian
    )
    external
    onlyFromWalletOrOwnerWhenUnlocked
{
    wallet.addGuardian(guardian);
}

function addGuardianWA(
    Approval calldata approval,
    address           guardian
    )
    external
    returns (bytes32 approvedHash)
{
    approvedHash = wallet.addGuardianWA(DOMAIN_SEPARATOR, approval, guardian);
}

function removeGuardian(
    address guardian
    )
    external
    onlyFromWalletOrOwnerWhenUnlocked
{
    wallet.removeGuardian(guardian);
}

 function removeGuardianWA(
    Approval calldata approval,
    address           guardian
    )
    external
    returns (bytes32 approvedHash)
{
    approvedHash = wallet.removeGuardianWA(DOMAIN_SEPARATOR, approval, guardian);
}

 function resetGuardians(
     address[] calldata newGuardians
     )
     external
     onlyFromWalletOrOwnerWhenUnlocked
 {
     wallet.resetGuardians(newGuardians);
 }

 function resetGuardiansWA(
     Approval  calldata approval,
     address[] calldata newGuardians
     )
     external
     returns (bytes32 approvedHash)
 {
     approvedHash = wallet.resetGuardiansWA(DOMAIN_SEPARATOR, approval, newGuardians);
 }

 function isGuardian(address addr, bool includePendingAddition)
     public
     view
     returns (bool)
 {
     return wallet.isGuardian(addr, includePendingAddition);
 }

 function getGuardians(bool includePendingAddition)
     public
     view
     returns (Guardian[] memory )
 {
     return GuardianLib.guardians(wallet, includePendingAddition);
 }

//
// Inheritance
//

function setInheritor(
    address inheritor,
    uint32  waitingPeriod
    )
    external
    onlyFromWalletOrOwnerWhenUnlocked
{
    wallet.setInheritor(inheritor, waitingPeriod);
}

function inherit(
    address newOwner
    )
    external
{
    wallet.inherit(newOwner);
}

//
// Lock
//

function lock()
    external
{
    wallet.lock();
}

function unlock(
    Approval calldata approval
    )
    external
    returns (bytes32 approvedHash)
{
    approvedHash = wallet.unlock(DOMAIN_SEPARATOR, approval);
}

//
// Quota
//

function changeDailyQuota(
    uint newQuota
    )
    external
    onlyFromWalletOrOwnerWhenUnlocked
{
    wallet.changeDailyQuota(newQuota);
}

function changeDailyQuotaWA(
    Approval calldata approval,
    uint              newQuota
    )
    external
    returns (bytes32 approvedHash)
{
    approvedHash = wallet.changeDailyQuotaWA(DOMAIN_SEPARATOR, approval, newQuota);
}

//
// MetaTx
//

function executeMetaTx(
    address to,
    uint    nonce,
    address gasToken,
    uint    gasPrice,
    uint    gasLimit,
    uint    gasOverhead,
    address feeRecipient,
    bool    requiresSuccess,
    bytes   calldata data,
    bytes   memory   signature
    )
    external
    returns (bool)
{
    MetaTxLib.MetaTx memory metaTx = MetaTxLib.MetaTx(
        to,
        nonce,
        gasToken,
        gasPrice,
        gasLimit,
        gasOverhead,
        feeRecipient,
        requiresSuccess,
        data,
        signature
    );

    return wallet.executeMetaTx(
        DOMAIN_SEPARATOR,
        priceOracle,
        metaTx
    );
}

function selfBatchCall(
    bytes[] calldata data
    )
    external
    onlyFromWalletOrOwnerWhenUnlocked
{
    wallet.selfBatchCall(data);
}

//
// Recover
//

function recover(
    Approval calldata  approval,
    address            newOwner,
    address[] calldata newGuardians
    )
    external
    returns (bytes32 approvedHash)
{
    approvedHash = wallet.recover(
        DOMAIN_SEPARATOR,
        approval,
        newOwner,
        newGuardians
    );
}

//
// Whitelist
//

function addToWhitelist(
    address addr
    )
    external
    onlyFromWalletOrOwnerWhenUnlocked
{
    wallet.addToWhitelist(addr);
}

function addToWhitelistWA(
    Approval calldata approval,
    address           addr
    )
    external
    returns (bytes32 approvedHash)
{
    approvedHash = wallet.addToWhitelistWA(
        DOMAIN_SEPARATOR,
        approval,
        addr
    );
}

function removeFromWhitelist(
    address addr
    )
    external
    onlyFromWalletOrOwnerWhenUnlocked
{
    wallet.removeFromWhitelist(addr);
}

function getWhitelistEffectiveTime(
    address addr
    )
    public
    view
    returns (uint)
{
    return wallet.whitelisted[addr];
}

function isWhitelisted(
    address addr
    )
    public
    view
    returns (bool) {
    return wallet.isAddressWhitelisted(addr);
}

//
// ERC20
//

function transferToken(
    address        token,
    address        to,
    uint           amount,
    bytes calldata logdata,
    bool           forceUseQuota
    )
    external
    onlyFromWalletOrOwnerWhenUnlocked
{
    wallet.transferToken(
        priceOracle,
        token,
        to,
        amount,
        logdata,
        forceUseQuota
    );
}

function transferTokenWA(
    Approval calldata approval,
    address           token,
    address           to,
    uint              amount,
    bytes    calldata logdata
    )
    external
    returns (bytes32 approvedHash)
{
    approvedHash = wallet.transferTokenWA(
        DOMAIN_SEPARATOR,
        approval,
        token,
        to,
        amount,
        logdata
    );
}

function callContract(
    address          to,
    uint             value,
    bytes   calldata data,
    bool             forceUseQuota
    )
    external
    onlyFromWalletOrOwnerWhenUnlocked
    returns (bytes memory)
{
    return wallet.callContract(
        priceOracle,
        to,
        value,
        data,
        forceUseQuota
    );
}

function callContractWA(
    Approval calldata approval,
    address           to,
    uint              value,
    bytes    calldata data
    )
    external
    returns (bytes32 approvedHash, bytes memory returnData)
{
    (approvedHash, returnData) = wallet.callContractWA(
        DOMAIN_SEPARATOR,
        approval,
        to,
        value,
        data
    );
}

function approveToken(
    address token,
    address to,
    uint    amount,
    bool    forceUseQuota
    )
    external
    onlyFromWalletOrOwnerWhenUnlocked
{
    wallet.approveToken(
        priceOracle,
        token,
        to,
        amount,
        forceUseQuota
    );
}

function approveTokenWA(
    Approval calldata approval,
    address           token,
    address           to,
    uint              amount
    )
    external
    returns (bytes32 approvedHash)
{
    approvedHash = wallet.approveTokenWA(
        DOMAIN_SEPARATOR,
        approval,
        token,
        to,
        amount
    );
}

function approveThenCallContract(
    address          token,
    address          to,
    uint             amount,
    uint             value,
    bytes   calldata data,
    bool             forceUseQuota
    )
    external
    onlyFromWalletOrOwnerWhenUnlocked
    returns (bytes memory)
{
    return wallet.approveThenCallContract(
        priceOracle,
        token,
        to,
        amount,
        value,
        data,
        forceUseQuota
    );
}

function approveThenCallContractWA(
    Approval calldata approval,
    address           token,
    address           to,
    uint              amount,
    uint              value,
    bytes    calldata data
    )
    external
    returns (bytes32 approvedHash, bytes memory returnData)
{
    (approvedHash, returnData) = wallet.approveThenCallContractWA(
        DOMAIN_SEPARATOR,
        approval,
        token,
        to,
        amount,
        value,
        data
    );
}

// ERC165
function supportsInterface(
    bytes4 interfaceId
    )
    external
    pure
    override
    returns (bool)
{
    return  interfaceId == type(ERC1271).interfaceId ||
            interfaceId == type(IERC165).interfaceId ||
            interfaceId == type(IERC721Receiver).interfaceId ||
            interfaceId == type(IERC1155Receiver).interfaceId;
}

}

Version:

0.8.3

Relevant log output:

['INFO:Slither:\x1b[92m', 'Initializable contract not found, the contract does not follow a standard initalization schema.', 'Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#initializable-is-missing\x1b[0m', 'INFO:Slither:\x1b[92m', 'SmartWallet (after_14160000/44logic0.sol#3184-3820) needs to be initialized by SmartWallet.initialize(address,address[],uint256,address,address,address,uint256) (after_14160000/44logic0.sol#3263-3295).', 'Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#initialize-function\x1b[0m', 'INFO:Slither:\x1b[91m', 'Different variables between SmartWallet (after_14160000/44logic0.sol#3184-3820) and WalletProxy (after_14160000/44proxy.sol#14-49)', '\t SmartWallet.DOMAIN_SEPARATOR (after_14160000/44logic0.sol#3196)', '\t WalletProxy.masterCopy (after_14160000/44proxy.sol#18)', 'Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#incorrect-variables-with-the-proxy\x1b[0m', 'INFO:Slither:3 findings, 12 detectors run', '']
after_14160000/45names

zowie951 avatar Jul 06 '22 18:07 zowie951

What command did you run and what's the issue (it's not clear to me what result you're saying is a false positive)?

0xalpharush avatar Jul 06 '22 18:07 0xalpharush

command: slither-check-upgradeability smartwallet.sol SmartWallet --proxy-filename proxy.sol --proxy-name WalletProxy

result: INFO:Slither: Initializable contract not found, the contract does not follow a standard initalization schema. Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#initializable-is-missing INFO:Slither: SmartWallet (after_14160000/332logic0.sol#3184-3820) needs to be initialized by SmartWallet.initialize(address,address[],uint256,address,address,address,uint256) (after_14160000/332logic0.sol#3263-3295). Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#initialize-function INFO:Slither: Different variables between SmartWallet (after_14160000/332logic0.sol#3184-3820) and WalletProxy (after_14160000/332proxy.sol#14-49) SmartWallet.DOMAIN_SEPARATOR (after_14160000/332logic0.sol#3196) WalletProxy.masterCopy (after_14160000/332proxy.sol#18) Reference: https://github.com/crytic/slither/wiki/Upgradeability-Checks#incorrect-variables-with-the-proxy INFO:Slither:3 findings, 12 detectors run

I think the immutable variable should not be seen as a state variable so there should not be a "Different variables" issue.

zowie951 avatar Jul 06 '22 18:07 zowie951

I believe this was fixed by https://github.com/crytic/slither/pull/1451

0xalpharush avatar Feb 24 '23 14:02 0xalpharush