Salty.IO - The-Seraphs's results

Lines of code

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

Vulnerability details

Description

The Pools::removeLiquidity() function removes liquidity for a user by updating the reserves. After the reserves are updated, it performs the following check to ensure that reserve0 and reserve1 are above PoolUtils.DUST.

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

However, the above check has a typo, and only checks that reserve0 is above PoolUtils.DUST. Due to this, the reserve1 can be made to go below the DUST amount.

Impact

Due to the absence of a validation check, the balance of the reserve1 can fall below the specified DUST threshold. This can potentially disrupt the proper functioning of the following functions, as well as any other external functions that depend on it, as they require the reserves to remain above the DUST threshold:

• Pools::swap()
• Pools::depositSwapWithdraw()
• Pools::depositDoubleSwapWithdraw()

Proof of concept

The test below can be placed in src/pools/tests/Pools.t.sol, and run using the command COVERAGE="yes" NETWORK="sep" forge test -vvv --rpc-url <RPC> --mt "test_poc_takeReserve1BelowDust"

    function test_poc_takeReserve1BelowDust() public {
        vm.startPrank(address(collateralAndLiquidity));

        (uint256 reserves0_1, uint256 reserves1_1) = pools.getPoolReserves(
            tokens[6],
            tokens[5]
        );
        assertEq(reserves0_1, 1000 ether);
        assertEq(reserves1_1, 1000 ether);

        pools.deposit(tokens[6], 2000 ether);

        // Swap to change the ratio of tokens so that reserve1 has less amount
        pools.swap(
            tokens[6],
            tokens[5],
            1000 ether,
            1 ether,
            block.timestamp + 60
        );

        (uint256 reserves0_2, uint256 reserves1_2) = pools.getPoolReserves(
            tokens[6],
            tokens[5]
        );
        assertEq(reserves0_2, 2000 ether);
        assertEq(reserves1_2, 500 ether);

        uint256 totalShares = collateralAndLiquidity.totalShares(
            PoolUtils._poolID(tokens[5], tokens[6])
        );

        // Remove enough liquidity to make reserve1 below DUST
        pools.removeLiquidity(
            tokens[6],
            tokens[5],
            ((reserves1_2 - (PoolUtils.DUST - 1)) * (reserves0_2/reserves1_2)), // ((500 ether - 99) * 4),
            1 ether,
            1 ether,
            totalShares
        );

        (uint256 reserves0_3, uint256 reserves1_3) = pools.getPoolReserves(
            tokens[6],
            tokens[5]
        );
        assertEq(reserves0_3, 396);
        assertLt(reserves1_3, PoolUtils.DUST);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Update the following check to ensure both reserve0 and reserve1 are being checked for the DUST amount.

//-----------------------------------------------------------------V
-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

[QA/Low-01] Use flipped in Pools::removeLiquidity() function so that the users do not have to order the tokens correctly in the function call, similar to how its done for all other functions

Code references

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

Description

The function Pools::removeLiquidity() retrieves the pool ID and if the token IDs are flipped.

(bytes32 poolID, bool flipped) = PoolUtils._poolIDAndFlipped(tokenA, tokenB);

The pool ID is determined as follows:

function _poolIDAndFlipped( IERC20 tokenA, IERC20 tokenB ) internal pure returns (bytes32 poolID, bool flipped)
{
    // See if the token orders are flipped
    if ( uint160(address(tokenB)) < uint160(address(tokenA)) )
        return (keccak256(abi.encodePacked(address(tokenB), address(tokenA))), true);

    return (keccak256(abi.encodePacked(address(tokenA), address(tokenB))), false);
}

Flipped is returned as true if tokenB < tokenA.

This ensures that the tokenA, tokenB and the corresponding amounts provided in the function call matches the tokens for reserve0 and reserve1 of the pool.

However, flipped is not used in the function at all, and its the user's responsibility to provide the tokens in the right order.

The other functions like Pools::addLiquidity() use flipped and flip the tokens if needed.

Recommendation

Use flipped in the code to flip the tokens if the user provides the tokens in the wrong order.

---

[QA/Low-02] Add checks in DAO::_executeSetContract() to ensure the same price feed is not set for different price feeds

Code references

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/DAO.sol#L135-L142

Description

In DAO::_executeSetContract(), based on the nameHash, the price feed is set to priceFeedNum.

if ( nameHash == keccak256(bytes( "setContract:priceFeed1_confirm" )) )
    priceAggregator.setPriceFeed( 1, IPriceFeed(ballot.address1) );
else if ( nameHash == keccak256(bytes( "setContract:priceFeed2_confirm" )) )
    priceAggregator.setPriceFeed( 2, IPriceFeed(ballot.address1) );
else if ( nameHash == keccak256(bytes( "setContract:priceFeed3_confirm" )) )
    priceAggregator.setPriceFeed( 3, IPriceFeed(ballot.address1) );

There are no checks here that ensure that the price feed is already not set for another priceFeedNum, that is, price feed 1, 2 and 3 all can point to the same price feed.

Recommendation

Add checks to ensure that price feed 1, 2 and 3 are always different.

---

[QA/Low-03] Use modifiers in DAO.sol contract instead of writing the same checks multiple times

Code references

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/DAO.sol#L297

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/DAO.sol#L318

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/DAO.sol#L329

Description

In DAO.sol, there are multiple functions that validate that only Upkeep contract calls the function.

require( msg.sender == address(exchangeConfig.upkeep()), "DAO.XYZ is only callable from the Upkeep contract" );

This check has been added multiple times in the functions in the contract.

This can be replaced by a modifier to make the code more readable, avoid mistakes in case the code needs update, and follow Do Not Repeat(DRY) principles to not repeat the same code multiple times

Recommendation

Add a modifier that performs the require check, and use them in the functions

---