Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protocol Insolvency Risk Due to Unchecked Asset Increases in Atom and Triple Creation #81

Open
hats-bug-reporter bot opened this issue Jun 30, 2024 · 1 comment
Labels
bug Something isn't working invalid This doesn't seem right

Comments

@hats-bug-reporter
Copy link

Github username: --
Twitter username: --
Submission hash (on-chain): 0x45af594e37907786efc19eea5046d4b897518936ba6dc1ee0e011e4b1bfa26dd
Severity: high

Description:
Description
In the EthMultiVault contract, there are two critical functions, _createTriple and _createAtom, that increase the total assets of vaults without ensuring that the protocol has sufficient ETH to back these increases. This can lead to a situation where the protocol becomes insolvent, unable to fulfill withdrawal requests if users attempt to redeem their shares.

  1. In _createTriple:

    • The function increases the total assets of each underlying atom vault by tripleConfig.atomDepositFractionOnTripleCreation / 3 without increasing the total shares or ensuring the protocol has these additional assets.
  2. In _createAtom:

    • The function deposits atomConfig.atomWalletInitialDepositAmount into the new atom vault for the atom wallet, without verifying if the protocol actually has these funds available.

These operations create a discrepancy between the recorded total assets and the actual ETH held by the protocol, potentially leading to insolvency.

Attack Scenario\

  1. An attacker observes that tripleConfig.atomDepositFractionOnTripleCreation is set to a non-zero value.
  2. The attacker creates multiple triples, each time increasing the total assets of the underlying atom vaults without actually providing these additional assets.
  3. Over time, this inflates the recorded total assets of the atom vaults beyond the actual ETH held by the protocol.
  4. When users try to withdraw their funds, the protocol may not have sufficient ETH to fulfill all withdrawal requests, leading to insolvency.

Similarly, for atom creation:

  1. An attacker creates multiple atoms, each time the protocol "deposits" atomConfig.atomWalletInitialDepositAmount into the new atom vault.
  2. If the protocol doesn't actually hold these funds, it creates a deficit that grows with each atom creation.
  3. This could be exploited to drain the protocol of its actual ETH holdings.

Attachments

// deposit atomWalletInitialDepositAmount amount of assets and mint the shares for the atom wallet
_depositOnVaultCreation(
id,
atomWallet, // receiver
atomConfig.atomWalletInitialDepositAmount

if (tripleConfig.atomDepositFractionOnTripleCreation > 0) {
for (uint256 i = 0; i < 3; i++) {
uint256 atomId = tripleAtomIds[i];
// increase the total assets in each underlying atom vault
_setVaultTotals(
atomId,
vaults[atomId].totalAssets + (tripleConfig.atomDepositFractionOnTripleCreation / 3),
vaults[atomId].totalShares
);
}

  1. Proof of Concept (PoC) File
function testProtocolInsolvencyRisk() external {
    vm.startPrank(alice, alice);

    // Create atoms
    uint256 testAtomCost = getAtomCost();
    uint256 subjectId = ethMultiVault.createAtom{value: testAtomCost}("subject");
    uint256 predicateId = ethMultiVault.createAtom{value: testAtomCost}("predicate");
    uint256 objectId = ethMultiVault.createAtom{value: testAtomCost}("object");

    // Create a triple
    uint256 tripleCost = getTripleCost();
    uint256 tripleId = ethMultiVault.createTriple{value: tripleCost}(subjectId, predicateId, objectId);

    vm.stopPrank();

    // Record initial balances
    uint256 initialProtocolBalance = address(ethMultiVault).balance;
    uint256 initialTotalAssets = vaultTotalAssets(subjectId) + vaultTotalAssets(predicateId) + vaultTotalAssets(objectId);

    // Set a non-zero atomDepositFractionOnTripleCreation
    vm.prank(address(ethMultiVault.generalConfig().admin));
    ethMultiVault.setAtomDepositFractionOnTripleCreation(1 ether);

    vm.startPrank(bob, bob);

    // Create another triple, which should increase atom vault balances
    ethMultiVault.createTriple{value: tripleCost}(subjectId, predicateId, objectId);

    vm.stopPrank();

    // Check final balances
    uint256 finalProtocolBalance = address(ethMultiVault).balance;
    uint256 finalTotalAssets = vaultTotalAssets(subjectId) + vaultTotalAssets(predicateId) + vaultTotalAssets(objectId);

    // Verify that recorded assets have increased more than actual ETH balance
    assertGt(finalTotalAssets - initialTotalAssets, finalProtocolBalance - initialProtocolBalance, "Protocol should show higher asset increase than actual ETH received");

    // Attempt to withdraw all assets from an atom vault
    vm.prank(alice);
    uint256 aliceShares = ethMultiVault.vaults(subjectId).balanceOf[alice];
    vm.expectRevert(); // Expect this to revert due to insufficient funds
    ethMultiVault.redeemAtom(aliceShares, alice, subjectId);
}
  1. Revised Code File (Optional)
contract EthMultiVault {
    uint256 public protocolOwnedAssets;

    function _createTriple(uint256 subjectId, uint256 predicateId, uint256 objectId, uint256 value) internal returns (uint256, uint256) {
        // ... existing code ...

        if (tripleConfig.atomDepositFractionOnTripleCreation > 0) {
            uint256 totalIncrease = tripleConfig.atomDepositFractionOnTripleCreation;
            require(protocolOwnedAssets >= totalIncrease, "Insufficient protocol assets");
            protocolOwnedAssets -= totalIncrease;

            for (uint256 i = 0; i < 3; i++) {
                uint256 atomId = tripleAtomIds[i];
                uint256 increase = totalIncrease / 3;
                _setVaultTotals(
                    atomId,
                    vaults[atomId].totalAssets + increase,
                    vaults[atomId].totalShares
                );
            }
        }

        // ... rest of the function ...
    }

    function _createAtom(bytes calldata atomUri, uint256 value) internal returns (uint256, uint256) {
        // ... existing code ...

        uint256 initialDeposit = atomConfig.atomWalletInitialDepositAmount;
        require(protocolOwnedAssets >= initialDeposit, "Insufficient protocol assets");
        protocolOwnedAssets -= initialDeposit;

        _depositOnVaultCreation(
            id,
            atomWallet,
            initialDeposit
        );

        // ... rest of the function ...
    }

    // New function to add protocol-owned assets
    function addProtocolAssets() external payable {
        protocolOwnedAssets += msg.value;
    }

    // ... other necessary changes ...
}

// Comments:
// 1. We've introduced a `protocolOwnedAssets` variable to track the protocol's actual ETH holdings.
// 2. Before increasing vault assets or making deposits, we check if the protocol has sufficient assets.
// 3. We deduct from `protocolOwnedAssets` when increasing vault balances or making initial atom wallet deposits.
// 4. A new function `addProtocolAssets` allows the protocol to add to its asset holdings.
// 5. This approach ensures that the protocol cannot become insolvent due to these operations.
// 6. Additional changes may be needed throughout the contract to maintain this balance accurately.
@hats-bug-reporter hats-bug-reporter bot added the bug Something isn't working label Jun 30, 2024
@mihailo-maksa mihailo-maksa added the invalid This doesn't seem right label Jul 1, 2024
@mihailo-maksa
Copy link
Collaborator

The report claims that _createTriple and _createAtom functions increase the total assets of vaults without ensuring the protocol has sufficient ETH to back these increases, potentially leading to protocol insolvency.

Label: invalid

Comment:
Claims about the protocol reaching insolvency levels are entirely invalid. The recommended check introducing the protocolOwnedAssets variable is redundant. In Intuition Protocol v1, there is no concept of "protocol-owned assets"; all assets and shares in all vaults are owned by their respective owners (users). It’s crucial to note that _createAtom and _createTriple cannot be considered in isolation since getAtomCost and getTripleCost are checked in their respective functions (createAtom and createTriple). These checks encompass the full values of atomConfig.atomWalletInitialDepositAmount and tripleConfig.atomDepositFractionOnTripleCreation, respectively. The entire function will fail on the first check if the user didn’t provide enough funds, making the described insolvency scenarios impossible. Finally, the provided PoC is completely invalid due to multiple reasons, one of which being the fact that the reporter is trying to create a duplicate triple, and is not accounting for counter triple vaults for asset comparisons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

1 participant