# A vulnerability in some contract wallet

I found a significant overlooked vulnerability in contract wallets:
use `mapping(address => address) LinkedArray` to store owners.
Let's examine the owner storage in some wallet contracts:

1. Safe wallet [link 1](https://github.com/safe-global/safe-contracts/blob/bf943f80fec5ac647159d26161446ac5d716a294/contracts/base/OwnerManager.sol#LL140C5-L142C6):

// link 1
mapping(address => address) internal owners;
function isOwner(address owner) public view returns (bool) {
        return owner != SENTINEL_OWNERS && owners[owner] != address(0);
}

2. Soulwallet [link 1](https://github.com/proofofsoulprotocol/soul-wallet-contract/blob/0cfac6478a8529c2179af343722489343f8e922f/contracts/base/OwnerManager.sol#L17) [link 2](https://github.com/proofofsoulprotocol/soul-wallet-contract/blob/0cfac6478a8529c2179af343722489343f8e922f/contracts/libraries/AddressLinkedList.sol#L83):

// link 1
function _isOwner(address addr) internal view override returns (bool) {
    return _ownerMapping().isExist(addr);
}

// link 2
function isExist(mapping(address => address) storage self, address addr) internal view onlyAddress(addr) returns (bool)
{
    return self[addr] != address(0);
}

In addition to the Safe wallet, there are other contract wallets based on Safe, such as Candidewallet. This type of wallet that relies directly on the Safe owner system without using multisig may face even greater risks than Safe.

As you may have noticed:
both `owners[owner] != address(0)` and `self[addr] != address(0)` cannot guarantee that `owner` or `addr` exists in the `mapping(address => address) LinkedArray`. The `isOwner()` and `isExist()` functions do not perform this check during execution (which would consume a significant amount of gas). This can lead to the addition of a "hidden" owner during any delegatecall, especially in a malicious context, where `sstore(bytes32, data)` is executed. In such cases, the pre-execution "simulation" ([link](https://twitter.com/jayden_sudo/status/1670092049587240960)) to ensure data safety will no longer be effective, as it cannot guarantee security unless code auditing ensures that the bytes32 in sstore comes from a valid source rather than a "black box".

To avoid this issue, we have a reason to consider abandoning LinkedArray and instead use a fixed-length owner structure, and read all the data sequentially from the first storage slot each time we read the owner, For example, we can use 10 consecutive adjacent storage slots to store owner information, allowing the wallet to support up to 16 owners (considering coding complexity and gas efficiency, supporting 10 owners would be more reasonable). However, the drawback is that the number of supported owners is fixed and relatively small.

Another implementation approach is to reserve a larger space for storing owners, such as 1M * bytes32. This would allow support for up to 1 million (1024 * 1024) owners. When adding an owner, the address would be modulo 1 million to determine which storage slot to use. This way, querying would only require one computation step and one storage read. However, this approach may result in storage conflicts, as only one owner with the same modulo value can be stored. We could increase the range from 1M to 1G to reduce the likelihood of storage conflicts, but it cannot be completely avoided.

If you have other ideas, please feel free to leave a comment on [twitter](https://twitter.com/jayden_sudo/status/1670505152489586688), or directly comment on this document.

more:

Thanks to Co-Founder@Safe Richard for the information！
1. That is pretty much what is described in this issue (https://github.com/safe-global/safe-contracts/issues/198)
2. OpenZeppelin wrote a couple blog posts to related topics (especially when it comes to delegatecall and modules) http://blog.openzeppelin.com/backdooring-gnosis-safe-multisig-wallets

- Update:
- For security reasons, the delegateCall support was removed: https://github.com/proofofsoulprotocol/soul-wallet-contract/pull/84
- Added a storage interface for plugins to use. Plugins that need to use a lot of dynamic data can be solved through this interface:

    interface IPluginStorage {
        function pluginDataStore(bytes32 key, bytes calldata value) external;
        function pluginDataLoad(address plugin, bytes32 key) external view returns (bytes memory);
}