Salty.IO - neocrao's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/ManagedWallet.sol#L73

Vulnerability details

Description

The ManagedWallet.sol has the following flow to update the wallet:

Step 1 - ManagedWallet::proposeWallets(): Current main wallet proposes new main wallet, and new confirmation wallet. If there is already a proposed new main wallet, then the proposal cannot be made

Step 2 - ManagedWallet::receive(): The confirmation wallet sends funds to confirm the wallet update. Time lock initiated.

Step 3 - ManagedWallet::changeWallets(): Upon timelock's expiration, the proposed main wallet confirms the wallet updates, and the wallets get updated in the contract

In Step 3, if the proposed main wallet never confirms the final wallet update, then no new proposals can ever be made in Step 1, as the proposed new wallet is still is proposed state.

Impact

If the proposed main wallet was incorrect, or if the access to the proposed main wallet is lost (lost private keys, contract destruction etc), then there is no way to propose a new wallet, and the Managed Wallet contract becomes useless.

Proof of concept

Use the below test in the file: src/root_tests/ManagedWallet.t.sol

function testNeoCraoPocWalletChangeStuck() public {
    address initialMainWallet = address(0x1111);
    address initialConfirmationWallet = address(0x2222);

    address newMainWallet = address(0x3333);
    address newConfirmationWallet = address(0x4444);

    address actualNewMainWallet = address(0x5555);
    address actualNewConfirmationWallet = address(0x6666);

    vm.deal(initialConfirmationWallet, 1 ether);

    ManagedWallet managedWallet = new ManagedWallet(initialMainWallet, initialConfirmationWallet);

    vm.startPrank(initialMainWallet);
    managedWallet.proposeWallets(newMainWallet, newConfirmationWallet);
    vm.stopPrank();

    uint256 sentValue = 0.06 ether;
    vm.prank(initialConfirmationWallet);
    (bool success,) = address(managedWallet).call{value: sentValue}("");
    assertTrue(success, "Confirmation of wallet proposal failed");

    uint256 currentTime = block.timestamp;
    vm.warp(currentTime + TIMELOCK_DURATION);

    vm.startPrank(initialMainWallet);
    vm.expectRevert("Cannot overwrite non-zero proposed mainWallet.");
    managedWallet.proposeWallets(actualNewMainWallet, actualNewConfirmationWallet);
    vm.stopPrank();

    // If newMainWallet never calls changeWallets(), then the wallets can never be updated
}

Severity Justification

This is Medium severity, based on the Code4rena Severity Categorization: https://docs.code4rena.com/awarding/judging-criteria/severity-categorization

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Tools Used

Manual Review

Recommended Mitigation Steps

After the Step 2 timelock expires, start another timelock within which the proposed main wallet should call ManagedWallet::changeWallets(). Beyond the expiration of this new timelock, new main wallet proposals should be allowed.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L187

Vulnerability details

Description

The function Pools::removeLiquidity() checks that the reserve0 and reserve1 amounts for a pool are always above or at DUST amount. If after the liquidity is removed, and the reserve0 or reserve1 goes below DUST level, then the transaction should revert.

But, this check is implemented incorrectly. The code that performs this check is as below in the code:

    require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

If you notice, the reserve0 is checked twice, instead of the checks being for each reserve0 and reserve1.

Impact

The documentation states the following reason for the DUST check:

// Make sure that removing liquidity doesn't drive either of the reserves below DUST.
// This is to ensure that ratios remain relatively constant even after a maximum withdrawal.

So, if the reserve1 goes below the DUST amount, then it can imbalance the ratios. Also, once a pool has been established with the reserves, functions like and related to swap() rely on the amounts to be above the DUST amounts. If either of the reserves go below the DUST levels, then these functions will start to revert.

Proof of concept

Use the below test in the file: src/pools/tests/Pools.t.sol

function testNeoCraoReserve1LessThanDust() public {
    // Setup Tokens

    TestERC20[] memory testTokens = new TestERC20[](2);

    vm.startPrank(DEPLOYER);
    for (uint256 i = 0; i < 2; i++) {
        testTokens[i] = new TestERC20("TEST", 18);
    }
    (, bool flipped) = PoolUtils._poolIDAndFlipped(testTokens[0], testTokens[1]);
    if (flipped) {
        (testTokens[1], testTokens[0]) = (testTokens[0], testTokens[1]);
    }
    for (uint256 i = 0; i < 2; i++) {
        testTokens[i].approve(address(pools), type(uint256).max);
        testTokens[i].approve(
            address(collateralAndLiquidity),
            type(uint256).max
        );

        testTokens[i].transfer(address(this), 100000 ether);
        testTokens[i].transfer(address(dao), 100000 ether);
        testTokens[i].transfer(address(collateralAndLiquidity), 100000 ether);
    }
    vm.stopPrank();

    vm.prank(address(dao));
    poolsConfig.whitelistPool(pools, testTokens[0], testTokens[1]);

    vm.prank(DEPLOYER);
    collateralAndLiquidity.depositLiquidityAndIncreaseShare(
        testTokens[0],
        testTokens[1],
        500 ether,
        100 ether,
        0,
        block.timestamp,
        false
    );

    for (uint256 i = 0; i < 2; i++) {
        testTokens[i].approve(address(pools), type(uint256).max);
        testTokens[i].approve(
            address(collateralAndLiquidity),
            type(uint256).max
        );
    }

    vm.startPrank(address(collateralAndLiquidity));
    testTokens[0].approve(address(pools), type(uint256).max);
    testTokens[1].approve(address(pools), type(uint256).max);

    // POC

    vm.startPrank(address(collateralAndLiquidity));

    uint256 totalShares = collateralAndLiquidity.totalShares(
        PoolUtils._poolID(testTokens[0], testTokens[1])
    );

    pools.removeLiquidity(
        testTokens[0],
        testTokens[1],
        599999999999999999406,
        1 ether,
        1 ether,
        totalShares
    );

    (, uint256 reserves1Final) = pools.getPoolReserves(
        testTokens[0],
        testTokens[1]
    );
    assertLt(reserves1Final, PoolUtils.DUST);
}

Severity Justification

This is Medium severity, based on the Code4rena Severity Categorization: https://docs.code4rena.com/awarding/judging-criteria/severity-categorization

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Tools Used

Manual Review

Recommended Mitigation Steps

The check should be updated to check for reserve1 as well.

-    require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");
+    require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve1 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

Assessed type

Invalid Validation