If a Triple vault is created with the AtomWallet as receiver, the minimum shares which should be minted to address(0) will remain stuck in the protocol

[hats-bug-reporter[bot]]

Github username: -- Twitter username: iamandreiski Submission hash (on-chain): 0x236e311ed5925325d69653b89a32194161c553b74ab2bf0daf8800e87149ba9e Severity: medium

Description: Description

If a malicious or otherwise just unbeknownst user creates a Triple vault with the Atom wallet as the receiver address, the minimum shares which are usually minted to address(0) as receiver to the particular vault id will not be minted and will remain "stuck" in the protcol since they were already accounted as part of the "fees" from the user's msg.value / deposit.

Attack Scenario

Since the address of the Atom wallet can be "predicted" / "computed" before its created since it's dependent solely on the vaultId, and the vaultId can be predicted as well since its id is just a nonce increment, an unbeknownst or just otherwise malicious user, can predict both values and "create" a Triple vault with the address of the Atom wallet as receiver.

The address of the atom wallet can be fetched by calling the computeAtomWalletAddr(id) function with the nonce that will be assigned to the newly created vault.

When createTriple is called with receiver as the yet-to-be-deployed Atom wallet, first _createTriple() will be called, in which the atom cost will be calculated and subtracted from the total msg.value so that it can be used later:

  uint256 userDeposit = value - getTripleCost();

        // create a new positive triple vault
        uint256 id = _createVault();

        // calculate protocol deposit fee
        uint256 protocolDepositFee = protocolFeeAmount(userDeposit, id);

        // calculate user deposit after protocol fees
        uint256 userDepositAfterprotocolFee = userDeposit - protocolDepositFee;

        // map the resultant triple hash to the new vault ID of the triple
        triplesByHash[hash] = id;

        // map the triple's vault ID to the underlying atom vault IDs
        triples[id] = [subjectId, predicateId, objectId];

        // set this new triple's vault ID as true in the IsTriple mapping as well as its counter
        isTriple[id] = true;

        uint256 atomDepositFraction = atomDepositFractionAmount(userDepositAfterprotocolFee, id);

        // give the user shares in the positive triple vault
        _depositOnVaultCreation(
            id,
            msg.sender, // receiver
            userDepositAfterprotocolFee - atomDepositFraction
        );

The triple cost is calculated in the following way:

function getTripleCost() public view returns (uint256) {
        uint256 tripleCost = tripleConfig.tripleCreationProtocolFee // paid to protocol
            + tripleConfig.atomDepositFractionOnTripleCreation // goes towards increasing the amount of assets in the underlying atom vaults
            + generalConfig.minShare * 2; // for purchasing ghost shares for the positive and counter triple vaults
        return tripleCost;
    }

And when later the _depositOnVaultCreation() is invoked, the following logic will take place:

 bool isAtomWallet = receiver == computeAtomWalletAddr(id);

        // ghost shares minted to the zero address upon vault creation
        uint256 sharesForZeroAddress = generalConfig.minShare;

        // ghost shares for the counter vault
        uint256 assetsForZeroAddressInCounterVault = generalConfig.minShare;

        uint256 sharesForReceiver = assets;

        // changes in vault's total assets or total shares
        uint256 totalDelta = isAtomWallet ? sharesForReceiver : sharesForReceiver + sharesForZeroAddress;

        // set vault totals for the vault
        _setVaultTotals(id, vaults[id].totalAssets + totalDelta, vaults[id].totalShares + totalDelta);

        // mint `sharesOwed` shares to sender factoring in fees
        _mint(receiver, id, sharesForReceiver);

        // mint `sharesForZeroAddress` shares to zero address to initialize the vault
        if (!isAtomWallet) {
            _mint(address(0), id, sharesForZeroAddress);
        }

        /// Initialize the counter triple vault with ghost shares if it is a triple creation flow
        if (isTripleId(id)) {
            uint256 counterVaultId = getCounterIdFromTriple(id);

            // set vault totals
            _setVaultTotals(
                counterVaultId,
                vaults[counterVaultId].totalAssets + assetsForZeroAddressInCounterVault,
                vaults[counterVaultId].totalShares + sharesForZeroAddress
            );

            // mint `sharesForZeroAddress` shares to zero address to initialize the vault
            _mint(address(0), counterVaultId, sharesForZeroAddress);
        }

Since the receiver will be the computed wallet address, one of the min shares will never be minted to the vault and attributed to address(0) due to the following which is related to the atom vault creation:

 // mint `sharesForZeroAddress` shares to zero address to initialize the vault
        if (!isAtomWallet) {
            _mint(address(0), id, sharesForZeroAddress);
        }

Since two mint share values were accounted for in the triple vault creation, one of them will be minted when the isTripleId logic is executed within the above-mentioned function, while the other value will remain "stuck" in the protocol since it was already subtracted from the shares which were minted to the user.

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

[mihailo-maksa]

The protocol is designed to mint a fixed small amount of shares to the address(0) to initialize the vault and prevent inflation attacks. These ghost shares are intentionally lost and allocated to the triple vault and counter vault.

Therefore, everything works as intended, and this issue is invalid.

[iamandreiski]

@mihailo-maksa That is true, but I think that there is a misunderstanding since it isn't what the issue is about.

The function _depositOnVaultCreation() is meant to be invoked when both Triple and Atom vaults are created. In case of Atom vaults, it has an exception in which if the receiver is the Atom wallet not to mint shares, i.e. if it isn't an Atom wallet then to mint ghost shares to address(0):

  if (!isAtomWallet) {
            _mint(address(0), id, sharesForZeroAddress);
        }

That functionality is predicted to be used during the creation of Atom vaults, but if the user uses a "atomWallet", i.e. predicts the address even though the Triple Atom Wallet doesn't exist, those shares will never get minted to the address(0), the only minting which will occur is for the ghost shares to counter vault. I.e. the min shares will be accounted and subtracted from the total amount, but they will never be used anywhere.

[mihailo-maksa]

Thanks for further clarifying your point, and sorry if I misinterpreted something. By our protocol design, it's not intended to deploy the atom wallets for the triple vaults, and we've since identified and fixed this issue. Please take a look at the commentary under the issue #55 for further details.

[iamandreiski]

Thank you for the response, appreciation for the further clarification.

I do understand your point but prior to the fix (when the codebase was being audited) the design as it was, it was allowed for that to take place since it was valid at that point in time. It may not be intended but it is plausible and a non-standard user path, which is a vulnerability by itself. Thank you!

[mihailo-maksa]

From the issue title it says: "If a Triple vault is created with the AtomWallet as receiver, the minimum shares which should be minted to address(0) will remain stuck in the protocol", but when looking at the createTriple function, there is no option to provide a receiver address, as all minted shares go straight to the msg.sender:

function createTriple(uint256 subjectId, uint256 predicateId, uint256 objectId) external payable nonReentrant whenNotPaused returns (uint256)

Therefore, this issue is invalid.