Saferoot Contracts - Staging Labs

The ContractRegistry contract is responsible for storing the addresses of the token contracts that are Saferoot verified. If this contract is disabled in the Saferoot contract, it can no longer be re-enabled. As a result, users could no longer use it to verify the token contracts when adding safeguards.

Code Location

If the registry is enabled in the Saferoot contract, it is used to verify the token contracts when adding safeguards:

contracts/Saferoot.sol

    if (
        registryEnabled &&
        !registry.isContractSupported(incomingSafeguard.contractAddress)
    ) {
        revert InvalidContractAddress();
    }

However, there is no enableRegistry function implemented in the Saferoot contract. The user can only disable the registry:

contracts/Saferoot.sol

    function disableRegistry() external onlyUser {
        registryEnabled = false;
        emit RegistryFlagUpdated(false);
    }

SOLVED: The \client team concluded that the contract registry was unnecessary and removed it from the protocol in commit 5166c1d.

The ContractRegistry contract is responsible for storing the addresses of the contracts that are Saferoot verified. Users may wish to use a different implementation, but once the Saferoot contract is initialized, it is no longer possible to update the registry address in it.

Code Location

contracts/Saferoot.sol

    function initialize(
        address _service,
        address _user,
        address _backup,
        address _contractRegistry,
        bool _registryEnabled
    ) public initializer {
        user = _user;
        backup = _backup;
        service = _service;
        contractRegistry = _contractRegistry;
        registryEnabled = _registryEnabled;
        _grantRole(SERVICE_ROLE, _service);
        _grantRole(USER_ROLE, _user);

        emit SaferootCreated(_user, _service);
    }

SOLVED: The \client team concluded that the contract registry was unnecessary and removed it from the protocol in commit 5166c1d.

The safeguards are used to transfer the tokens from the user to the backup wallet address if they are initiated. It was identified that users can create safeguards with parameters that are not related to their token type. For example, it is possible to create an ERC20 token safeguard with a non-zero tokenId parameter. Using an invalid parameter indicates that the user may have accidentally switched the order of the values. Reverting these function makes it easier to detect such errors.

Code Location

The SafeguardEntry and SafeguardEditEntry structs are used in the addSafeguard and editSafeguard functions:

contracts/Saferoot.sol

    struct SafeguardEntry {
        TokenType tokenType;
        address contractAddress;
        uint256 amount;
        uint256 tokenId;
    }

contracts/Saferoot.sol

    struct SafeguardEditEntry {
        bytes32 key;
        uint256 newTokenId;
        uint256 newAmount;
    }

For example, the tokenId parameter is not checked in the ERC20 branch of the addSafeguard function:

contracts/Saferoot.sol

            SafeguardEntry memory incomingSafeguard = _ercEntries[index];
            if (incomingSafeguard.contractAddress == address(0)) {
                revert ZeroAddress();
            }

            // If registry is enabled and the contract is a safe + supported address
            if (
                registryEnabled &&
                !registry.isContractSupported(incomingSafeguard.contractAddress)
            ) {
                revert InvalidContractAddress();
            }

            bytes32 key = encodeKey(currentKey, incomingSafeguard.tokenType);
            if (incomingSafeguard.tokenType == TokenType.ERC20) {
                ERC20SafeguardInfo storage safeguard = erc20Safeguards[key];
                if (safeguard.contractAddress == address(0)) {
                    safeguard.contractAddress = incomingSafeguard
                        .contractAddress;
                    safeguard.amount = incomingSafeguard.amount;
                    emit ERC20SafeguardAdded(key);
                    unchecked {
                        ++currentKey;
                    }
                }
            }

SOLVED: The \client team solved the issue in commits 6562223 and bbe3474 by reverting the related functions if the safeguards contain inappropriate values. The editSafeguard function was removed from the contract.

The Saferoot contract uses the Initializable module from OpenZeppelin, and the implementations of this contract are not marked as initialized in the constructor. An uninitialized instance can be initialized by anyone to take over the contract. Even if it does not affect the instances deployed by the SaferootFactory, it is still a good practice to prevent the initialization of the implementation contracts to avoid misuse. In the latest versions, this is done by calling the _disableInitializers function in the constructor.

SOLVED: The \client team solved the issue in commit d1df49e by including a constructor to automatically mark the contracts as initialized.

It was identified that there is no limit in the Saferoot contract on how many safeguards can be added by the user. Adding too many safeguards may prevent the service user to efficiently monitoring the system and call safeguards in time.

Note that the off-chain backend system was outside this security assessment's scope.

ACKNOWLEDGED: The \client team acknowledged this finding and will perform tests to identify the limitations of the service and manage the number of active safeguards off-chain.

It was identified that the Saferoot contract may not be compatible with tokens that do not follow the ERC standards if these tokens do not implement the functions (e.g., getApproved, safeTransferFrom) that the Saferoot contract uses to verify permissions for token transfers or transfer tokens to the backup address (e.g., CryptoKitties and CryptoPunks NFTs).

ACKNOWLEDGED: The \client team acknowledged this finding. They are going to create an off-chain database with the supported tokens, and they will notify the users about these limitations.

It was identified that multiple safeguards can be added to the same asset in the Saferoot contract. Adding multiple safeguards for the same asset may prevent the backend systems from correctly identifying which safeguards should be called.

Note that the backend systems were outside this security assessment's scope.

SOLVED: The \client team solved the issue in commit 837c806 by modifying the design of the Saferoot contract.

The initiateSafeguard function transfers the tokens from the user's wallet to the backup address based on the added safeguards. The keys of the safeguards to use are given in the function's _safeguardKeys array parameter. The function iterates through these keys and gets the data of the safeguards from the associated mappings, transferring the specified tokens from the user to the backup address. However, this involves looping over multiple array items, querying the required data, calling external contracts, and emitting events.

The execution of these actions requires a certain amount of gas based on how much computation is needed to complete them. Adding too many keys to the _safeguardKeys parameter may max out the gas limit, reverting the initiateSafeguard function, which prevents transferring the user's tokens in time.

Code Location

The initiateSafeguard function is looping over its dynamic array parameter:

contracts/Saferoot.sol

    function initiateSafeguard(bytes32[] calldata _safeguardKeys)
        external
        payable
        onlyService
        nonReentrant
    {
        for (uint256 index = 0; index < _safeguardKeys.length; ++index) {
            bytes32 key = _safeguardKeys[index];
            TokenType tokenType = decodeKeyTokenType(key);

            if (tokenType == TokenType.ERC20) {
                ERC20SafeguardInfo memory safeguard = erc20Safeguards[key];
                address contractAddress = safeguard.contractAddress;

                if (contractAddress != address(0)) {
                    // Transfer token
                    if (_transfer20(key, contractAddress, safeguard.amount)) {
                        emit SafeguardInitiated(key);
                    }
                    // If the contract address is already set, then skip
                    continue;
                }
            }

ACKNOWLEDGED: The \client team acknowledged this finding.

It was identified that the Saferoot, SaferootFactory, and ContractRegistry contracts are missing functions to sweep/recover accidental ERC20 token and Ether transfers. Mistakenly sent tokens and Ether are locked in the contracts indefinitely.

SOLVED: The \client team solved the issue in commit 910d794 by adding a function to enable the user to recover accidental ERC20 token transfers and removing the payable functions to prevent accidental Ether transfers. Note that only the user can withdraw accidental token transfers from the Saferoot contract.

It was identified that the for loops employed in the contracts can be gas optimized by the following principles:

• Unnecessary reading of the array length on each iteration wastes gas.

• A postfix (e.g. i++) operator was used to increment the i variables. It is known that, in loops, using prefix operators (e.g. ++i) costs less gas per iteration than postfix operators. It is also possible to further optimize loops by using unchecked loop index incrementing and decrementing.

Note that view or pure functions only cost gas if they are called from on-chain.

Code Location

contracts/ContractRegistry.sol

• Line 52: for (uint256 i = 0; i < _contracts.length; i++) {

contracts/Saferoot.sol

• Line 249: for (uint256 index = 0; index < _safeguardKeys.length; ++index) {
• Line 321: for (uint256 index; index < _ercEntries.length; ++index) {
• Line 392: for (uint256 index = 0; index < _ercEditEntries.length; ++index) {

The original and the optimized gas cost of the updateSupportedContracts function were compared to measure its gas efficiency:

Original: 162236

Optimized: 161833

Difference: 403

SOLVED: The \client team solved the issue in commit 0914a6b by applying the above recommendations.

It was identified that the addSafeguard function in the Saferoot contract employs unnecessary validation to check if the currentKey is already used or not. However, because the currentKey is increased every time a safeguard is added, this condition is always true.

Code Location

contracts/Saferoot.sol

    bytes32 key = encodeKey(currentKey, incomingSafeguard.tokenType);
    if (incomingSafeguard.tokenType == TokenType.ERC20) {
        ERC20SafeguardInfo storage safeguard = erc20Safeguards[key];
        if (safeguard.contractAddress == address(0)) {
            safeguard.contractAddress = incomingSafeguard
                .contractAddress;
            safeguard.amount = incomingSafeguard.amount;
            emit ERC20SafeguardAdded(key);
            unchecked {
                ++currentKey;
            }
        }
    } else if (incomingSafeguard.tokenType == TokenType.ERC721) {
        ERC721SafeguardInfo storage safeguard = erc721Safeguards[key];
        if (safeguard.contractAddress == address(0)) {
            safeguard.contractAddress = incomingSafeguard
                .contractAddress;
            safeguard.tokenId = incomingSafeguard.tokenId;
            emit ERC721SafeguardAdded(key);
            unchecked {
                ++currentKey;
            }
        }
    } else if (incomingSafeguard.tokenType == TokenType.ERC1155) {
        ERC1155SafeguardInfo storage safeguard = erc1155Safeguards[key];
        if (safeguard.contractAddress == address(0)) {
            safeguard.contractAddress = incomingSafeguard
                .contractAddress;
            safeguard.tokenId = incomingSafeguard.tokenId;
            safeguard.amount = incomingSafeguard.amount;
            emit ERC1155SafeguardAdded(key);
            unchecked {
                ++currentKey;
            }
        }
    }

SOLVED: The \client team solved the issue in commit bc0e505 by removing the unnecessary checks from the addSafeguard function.

It was identified that the contracts are using Solidity version 0.8.20 with its default Shanghai EVM version. The Shanghai fork introduced the PUSH0 opcode and was only supported on Mainnet, Goerli and Sepolia at the time of the assessment.

ACKNOWLEDGED: The \client team acknowledged this finding. The \client team will adjust the EVM version when deploying to new ecosystems.