Salty.IO - 0xSmartContractSamurai's results

Lines of code

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

Vulnerability details

Impact

• existing protocol tests did not catch this bug, despite efforts...good idea to modify the tests accordingly...

• It seems that this bug can never lead to any issues due to the proportionality factor remaining consistent between reclaimedA and reclaimedB regarding the values for reserve0 and reserve1, which means that neither of the reserves can ever become less than PoolUtils.DUST without the other reserve also becoming less than PoolUtils.DUST at the same time, i.e. it's impossible to have a situation where reserve1 < PoolUtils.DUST but reserve0 >= PoolUtils.DUST. (correct me if I'm wrong, but I checked).

• This means that IF reserve1 < PoolUtils.DUST ever happens, due to the bug this will be missed but due to above proportionality factor this reserve0 < PoolUtils.DUST will also happen and thankfully there's a check to prevent this one, so it will revert. (So it's basically accidental mitigation against any side-effects of this bug...)

• However, it's still a bug, and it needs to be fixed, the damage/impact on the protocol could have been serious if it wasnt for the accidental mitigation logic that prevents this (rather obvious) bug from doing anything...

Description:

The bug is in the require statement of the removeLiquidity function, where the condition (reserves.reserve0 >= PoolUtils.DUST) is duplicated, and condition (reserves.reserve1 >= PoolUtils.DUST) is skipped completely, effectively disregarding the value of reserves.reserve1. This oversight could have led to the function proceeding with liquidity removal and token transfers even when reserve1 falls below an acceptable level, however, it seems the way the logic in the function was implemented the potentially bad effects of this bug was side-stepped by pure luck. See impact section for more clarity on this.

The code snippet containing the bug: L187: https://github.com/code-423n4/2024-01-salty/blob/ce719503b43ebf086f5147c0f6efc3c0890bf0f9/src/pools/Pools.sol#L186-L187

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

Severity:

Definitely cannot be low severity due to the potential serious impact on entire protocol stability IF the function's logic didnt accidentally neutralize this bug. Therefore I deem the bug at least medium severity, but not high due to the fact the bug is neutralized accidentally.

However, since the bug wasnt picked up by the protocol's tests, exactly due to this accidental neutralization of the bug by the function's logic, it should serve as a good lesson to both devs and smart contract security auditoors. Exactly what that lesson is, I'm not sure, but successful tests are never a guarantee, as is the case here...

This (pretty obvious) bug was completely missed by all the tests, purely because it was impossible to detect it due to how the function was implemented, and by pure luck this implementation also accidentally neutralized the bug...

Proof of Concept

My PoC proves the bug, but since I struggled to setup a working test for locally and since I couldn't modify the contract on sepolia testnet for existing tests, I had to copy the buggy line of code from the removeLiquidity() function and add it to one of the existing protocol tests, in order to demonstrate the bug, what happens.

This should suffice as a proof of concept that demonstrates the bug.

Also, the existing protocol tests were not able to detect this bug due to the accidental bug neutralizing logic that was implemented in the function. Good lesson in here!

The accidental bug neutralizing logic:

		reclaimedA = ( reserves.reserve0 * liquidityToRemove ) / totalLiquidity;
		reclaimedB = ( reserves.reserve1 * liquidityToRemove ) / totalLiquidity;

		reserves.reserve0 -= uint128(reclaimedA);
		reserves.reserve1 -= uint128(reclaimedB);

Proof of Concept (PoC):

Existing test function's modification: Pools2.t.sol::testMaximumLiquidityRemoval()

    // A unit test to check for proper handling of maximum liquidity removal in removeLiquidity.
    function testMaximumLiquidityRemoval() public {
		vm.startPrank(address(collateralAndLiquidity));
		IERC20 tokenA = new TestERC20( "TEST", 18 );
		IERC20 tokenB = new TestERC20( "TEST", 18 );
		vm.stopPrank();

		vm.prank(address(dao));
		poolsConfig.whitelistPool( pools, tokenA, tokenB);

		vm.startPrank(address(collateralAndLiquidity));
		tokenA.approve(address(pools), type(uint256).max);
		tokenB.approve(address(pools), type(uint256).max);

		pools.addLiquidity(tokenA, tokenB, 100 ether, 100 ether, 0, 0);

		uint256 totalShares = 200 ether;
        pools.removeLiquidity(tokenA, tokenB, 200 ether * ( 100 ether - 100 ) / ( 100 ether ), 0, 0, totalShares);

		(uint256 reservesA, uint256 reservesB) = pools.getPoolReserves(tokenA, tokenB);

		// Ensure that DUST remains
		// assertEq( reservesA, PoolUtils.DUST ); /// @audit commented out for PoC
		// assertEq( reservesB, PoolUtils.DUST ); /// @audit commented out for PoC

+       /// @audit added for PoC testing purposes:
+       reservesB = 0; /// @audit invalid value should trigger revert, but bug avoids revert
+       //require((reservesA >= PoolUtils.DUST) && (reservesA >= PoolUtils.DUST), "Insufficient reserves after liquidity removal"); /// @audit bug avoids revert
+       require((reservesA >= PoolUtils.DUST) && (reservesB >= PoolUtils.DUST), "Insufficient reserves after liquidity removal"); /// @audit bug fixed: reverts as intended/expected

		// Ensure that liquidity can be added back in
		pools.addLiquidity(tokenA, tokenB, 100 ether, 100 ether, 0, 0);
		}

forge test command used: COVERAGE="no" NETWORK="sep" forge test --contracts src/pools/tests/Pools2.t.sol --mt testMaximumLiquidityRemoval -vvvvv --rpc-url https://eth-sepolia.g.alchemy.com/v2/gnLVzN-JjRB_L1mwqUL_LFNcyNBrpzRI

Case (bug not fixed):

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

Using for test:

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

Test results:

emit LiquidityRemoved(tokenA: TestERC20: [0x3a8D553652AdC9BdBB8c7212580C13c0C33a81eB], tokenB: TestERC20: [0x64f7F003bcDf4fc7625ff5e9312aa2e8b4bdd39F], reclaimedA: 99999999999999999900 [9.999e19], reclaimedB: 99999999999999999900 [9.999e19], removedLiquidity: 199999999999999999800 [1.999e20])
    │   └─ ← 99999999999999999900 [9.999e19], 99999999999999999900 [9.999e19]
    ├─ [1178] 0xce360D2cDCA292aD704223B145926336bf7a34Fb::getPoolReserves(TestERC20: [0x3a8D553652AdC9BdBB8c7212580C13c0C33a81eB], TestERC20: [0x64f7F003bcDf4fc7625ff5e9312aa2e8b4bdd39F]) [staticcall]
    │   └─ ← 100, 100
    ├─ [18550] 0xce360D2cDCA292aD704223B145926336bf7a34Fb::addLiquidity(TestERC20: [0x3a8D553652AdC9BdBB8c7212580C13c0C33a81eB], TestERC20: [0x64f7F003bcDf4fc7625ff5e9312aa2e8b4bdd39F], 100000000000000000000 [1e20], 100000000000000000000 [1e20], 0, 0)
    │   ├─ [3420] TestERC20::transferFrom(0x9Ecf36320532d700440323A16b1A86251a49B7e2, 0xce360D2cDCA292aD704223B145926336bf7a34Fb, 100000000000000000000 [1e20])
    │   │   ├─ emit Transfer(from: 0x9Ecf36320532d700440323A16b1A86251a49B7e2, to: 0xce360D2cDCA292aD704223B145926336bf7a34Fb, value: 100000000000000000000 [1e20])
    │   │   └─ ← true
    │   ├─ [3420] TestERC20::transferFrom(0x9Ecf36320532d700440323A16b1A86251a49B7e2, 0xce360D2cDCA292aD704223B145926336bf7a34Fb, 100000000000000000000 [1e20])
    │   │   ├─ emit Transfer(from: 0x9Ecf36320532d700440323A16b1A86251a49B7e2, to: 0xce360D2cDCA292aD704223B145926336bf7a34Fb, value: 100000000000000000000 [1e20])
    │   │   └─ ← true
    │   ├─ emit LiquidityAdded(tokenA: TestERC20: [0x3a8D553652AdC9BdBB8c7212580C13c0C33a81eB], tokenB: TestERC20: [0x64f7F003bcDf4fc7625ff5e9312aa2e8b4bdd39F], addedAmountA: 100000000000000000000 [1e20], addedAmountB: 100000000000000000000 [1e20], addedLiquidity: 0)
    │   └─ ← 100000000000000000000 [1e20], 100000000000000000000 [1e20], 0
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 119.45s
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Case (bug fixed):

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

Using for test:

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

Test results:

emit LiquidityRemoved(tokenA: TestERC20: [0x3a8D553652AdC9BdBB8c7212580C13c0C33a81eB], tokenB: TestERC20: [0x64f7F003bcDf4fc7625ff5e9312aa2e8b4bdd39F], reclaimedA: 99999999999999999900 [9.999e19], reclaimedB: 99999999999999999900 [9.999e19], removedLiquidity: 199999999999999999800 [1.999e20])
    │   └─ ← 99999999999999999900 [9.999e19], 99999999999999999900 [9.999e19]
    ├─ [1178] 0xce360D2cDCA292aD704223B145926336bf7a34Fb::getPoolReserves(TestERC20: [0x3a8D553652AdC9BdBB8c7212580C13c0C33a81eB], TestERC20: [0x64f7F003bcDf4fc7625ff5e9312aa2e8b4bdd39F]) [staticcall]
    │   └─ ← 100, 100
    └─ ← revert: Insufficient reserves after liquidity removal

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 111.65s
 
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in src/pools/tests/Pools2.t.sol:TestPools2
[FAIL. Reason: revert: Insufficient reserves after liquidity removal] testMaximumLiquidityRemoval() (gas: 2098967)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

VSC. Manual. Loads of frustration & persistence.

Recommended Mitigation Steps

Fix:

-       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");

Existing non-modified removeLiquidity() function with the bug:

	function removeLiquidity( IERC20 tokenA, IERC20 tokenB, uint256 liquidityToRemove, uint256 minReclaimedA, uint256 minReclaimedB, uint256 totalLiquidity ) external nonReentrant returns (uint256 reclaimedA, uint256 reclaimedB)
		{
		require( msg.sender == address(collateralAndLiquidity), "Pools.removeLiquidity is only callable from the CollateralAndLiquidity contract" );
		require( liquidityToRemove > 0, "The amount of liquidityToRemove cannot be zero" );

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

		// Determine how much liquidity is being withdrawn and round down in favor of the protocol
		PoolReserves storage reserves = _poolReserves[poolID];
		reclaimedA = ( reserves.reserve0 * liquidityToRemove ) / totalLiquidity;
		reclaimedB = ( reserves.reserve1 * liquidityToRemove ) / totalLiquidity;

		reserves.reserve0 -= uint128(reclaimedA);
		reserves.reserve1 -= uint128(reclaimedB);

		// 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.
        require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

		// Switch reclaimed amounts back to the order that was specified in the call arguments so they make sense to the caller
		if (flipped)
			(reclaimedA,reclaimedB) = (reclaimedB,reclaimedA);

		require( (reclaimedA >= minReclaimedA) && (reclaimedB >= minReclaimedB), "Insufficient underlying tokens returned" );

		// Send the reclaimed tokens to the user
		tokenA.safeTransfer( msg.sender, reclaimedA );
		tokenB.safeTransfer( msg.sender, reclaimedB );

		emit LiquidityRemoved(tokenA, tokenB, reclaimedA, reclaimedB, liquidityToRemove);
		}

Assessed type

Invalid Validation

0. Proposals::proposeSendSALT() - L202: Whenever salt balance of DAO.sol is 0 < DAO salt balance < 20, maxSendable will round down to zero, therefore 0 salt will be sent even when salt balance is 19.99.

https://github.com/code-423n4/2024-01-salty/blob/ce719503b43ebf086f5147c0f6efc3c0890bf0f9/src/dao/Proposals.sol#L202

Whenever exchangeConfig.salt().balanceOf( address(exchangeConfig.dao()) ) is > 0 but < 20, maxSendable will round down to zero, and therefore user passed parameter amount (which should be > 0) will trigger a revert in require( amount <= maxSendable, "Cannot send more than 5% of the DAO SALT balance" );, therefore wont be possible to send salt to anyone whenever DAO has salt tokens > 0 but < 20.

Impact:

• should be minimal on average
• approved proposal for sending salt reverting
• approved multi-proposal reverting due to salt balance less than 20, this could be more serious

PoC:

L202: maxSendable = balance * 5 / 100 Will round down to zero for balance * 5 / 100 < 1: balance * 5 / 100 < 1 balance < 100/5 balance < 20

So for DAO salt balance < 20, e.g. 19.99, maxSendable will round down to zero due to how solidity/evm treats non-integer numbers, especially anything less than 1.

Recommendation:

Might be OK if DAO salt balance will never be less than 20, otherwise:

• maybe leverage Fixed-Point Arithmetic library

=======

1. DAO::_executeApproval() - L170: ( exchangeConfig.salt().balanceOf(address(this)) >= ballot.number1 ) should use > instead of >= because of the related check in Proposals::proposeSendSALT() at L203: require( amount <= maxSendable.

https://github.com/code-423n4/2024-01-salty/blob/ce719503b43ebf086f5147c0f6efc3c0890bf0f9/src/dao/Proposals.sol#L203 https://github.com/code-423n4/2024-01-salty/blob/ce719503b43ebf086f5147c0f6efc3c0890bf0f9/src/dao/DAO.sol#L170

_executeApproval() L170:

if ( exchangeConfig.salt().balanceOf(address(this)) >= ballot.number1 )

Should be > due to check in Proposals::proposeSendSALT() and in line with this comment too: "Only one sendSALT Ballot can be open at a time and the sending limit is 5% of the current SALT balance of the DAO."

Impact:

• minimal, but more logically intact code, and in line with the correct logic in Proposals::proposeSendSALT() at L203.

Recommendation:

So, L170 should ideally be <= salt.balanceOf(address(this)) * 5 / 100; as follows:

if ( exchangeConfig.salt().balanceOf(address(this)) * 5 / 100 >= ballot.number1 )

=======

2. LOW: ArbitrageSearch::_rightMoreProfitable() - L68: potential rounding down to zero risk in uint256 amountOut = (reservesA1 * midpoint) / (reservesA0 + midpoint);

Chances of this happening is small, but not impossible, should be some extreme market or other external event which causes the initial reserves of 1000 to drop to updated reserves of 20, and with midpoint of 50 would cause amountOut to be rounded down to zero, effectively causing user to lose their funds during the swap/trade.

IMPACT:

• high, user will lose their funds used in the swap/trade
• chance of happening pretty low, due to extreme conditions/manipulation required. flashloan maybe?

PoC:

uint256 amountOut = (reservesA1 * midpoint) / (reservesA0 + midpoint);

For rounding down to zero we need the following condition to be true: reservesA1 * midpoint < reservesA0 + midpoint So: = reservesA1 < (reservesA0 + midpoint) / midpoint = reservesA1 < (reservesA0 / midpoint) + 1

Then when we apply this specific example: reservesA0 = 1000, reservesA1 = 20, and midpoint = 50, the condition for rounding down to zero is met. However, such extreme reductions in reserves from 1000 to 20 are unrealistic in typical AMM scenarios and may indicate abnormal behavior, a potential issue, or an attack on the system.

Completing the above equation: = 20 < 21 So if we go back to the original equation for amountOut: amountOut = (reservesA1 * midpoint) / (reservesA0 + midpoint) = (20 * 50) / (1000 + 50) = 0.952380952381 which will be rounded down to: = 0

=======

3. SigningTools::_verifySignature() - L26: Recommend to use OpenZeppelin's latest ECDSA over ecrecover() to enhance signature security and mitigate malleability risks.

https://github.com/code-423n4/2024-01-salty/blob/ce719503b43ebf086f5147c0f6efc3c0890bf0f9/src/SigningTools.sol#L26

(Although it may not pose any risk for this specific implementation due to the fact that the signature is expected to be reused, the recommendation remains valid.)

L26:

address recoveredAddress = ecrecover(messageHash, v, r, s);

RECOMMENDATION:

When it comes to smart contract signatures, consider swapping out ecrecover() for OpenZeppelin's ECDSA. Why? Well, certain signature schemes, like ECDSA, can get tricky with signature malleability, allowing the creation of distinct valid signatures for the same message and private key. OpenZeppelin's ECDSA library steps in as an advanced alternative, reducing the risk of encountering multiple valid signatures tied to a single message and key. In a nutshell, making this switch enhances signature security in smart contracts by addressing malleability challenges.

It's noteworthy that the missing check for a returned address(0) is not considered a concern, given the subsequent boolean "check" on L28 that effectively covers this scenario.

=======

4. SigningTools::_verifySignature() - L13: The constant 65 in the require statement require(signature.length == 65, "Invalid signature length"); is a magic number.

https://github.com/code-423n4/2024-01-salty/blob/ce719503b43ebf086f5147c0f6efc3c0890bf0f9/src/SigningTools.sol#L13

Consider replacing it with a named constant or providing a comment explaining why the length is expected to be 65.

=======

5. QA: CoreChainlinkFeed::latestChainlinkPrice() - L32: Replace deprecated Chainlink functions like latestRoundData() with current recommended methods for accurate oracle data retrieval in smart contracts.

https://github.com/code-423n4/2024-01-salty/blob/ce719503b43ebf086f5147c0f6efc3c0890bf0f9/src/price_feed/CoreChainlinkFeed.sol#L32

Using outdated Chainlink functions like latestRoundData() might serve up a bit of stale or wonky data, messing with the mojo of the contracts and price oracle functionality.

Reference: https://github.com/code-423n4/2022-04-backd-findings/issues/17

Good effort by the dev to try implement mitigations against the issues caused by latestRoundData(): https://github.com/code-423n4/2024-01-salty/blob/ce719503b43ebf086f5147c0f6efc3c0890bf0f9/src/price_feed/CoreChainlinkFeed.sol#L26-L60

Recommendation:

Despite the dev's efforts to address concerns with the deprecated latestRoundData method, opting for the recommended, up-to-date Chainlink practices is advisable. This ensures a proactive and reliable approach, aligning with current standards and reducing reliance on outdated functionality for long-term stability.

=======

6. QA: PriceAggregator::setPriceFeed() - L47: Risk of 35-day reliance on a single price feed if two feeds fail simultaneously, as setPriceFeed() can only be called once every 35 days for performance review.

https://github.com/code-423n4/2024-01-salty/blob/ce719503b43ebf086f5147c0f6efc3c0890bf0f9/src/price_feed/PriceAggregator.sol#L47

Potential problem if 2 pricefeeds go down at similar time and become unreliable for longer than acceptable, then stuck with just 1 pricefeed for at least 35 days, unless emergency action is implemented for such a case:

Comment on L11: "setPriceFeed can only be called once every 35 days by default (to allow time to review performance of the most recently upgraded PriceFeed before setting another)."

=======

7. QA/LOW: DAO::_executeSetContract() - L133: Missing input validation check for zero hash: if for whatever reason nameHash = 0x then event SetContract() will still be emitted.

https://github.com/code-423n4/2024-01-salty/blob/ce719503b43ebf086f5147c0f6efc3c0890bf0f9/src/dao/DAO.sol#L131-L145

Due to missing input validation if nameHash = 0x then the if block will be skipped and successfully emit event SetContract(ballot.ballotName, ballot.address1), which would be unexpected and potentially misleading, besides the fact that the function won't revert as it should and could potentially cause problems down the line.

Chances of nameHash = 0x happening is probably minimal but unless gas optimization is a valid excuse, the relevant input validation check should be implemented just to play it safe.

Recommendation:

	function _executeSetContract( Ballot memory ballot ) internal
		{
		bytes32 nameHash = keccak256(bytes( ballot.ballotName ) );
+		require(nameHash != 0x, "Invalid/zero hash");

=======

8. QA: Throughout the in-scope contracts there are several state variables which might contain sensitive/private/confidential data that use the private modifier, which can be viewed by anyone.

Blockchain data, even information designated as private within smart contracts, is susceptible to visibility by individuals skilled in querying the blockchain's state or analyzing transaction history. Private variables, despite their designation, are not immune to public scrutiny.

Recommendation:

To address this potential issue, it is recommended to either store sensitive data off-chain or encrypt it before being stored on-chain. Extra care should be taken to prevent on-chain data from inadvertently exposing private information.

=======

9. QA: InitialDistribution::distributionApproved() - L53: If for whatever reason salt.balanceOf(address(this)) gets skewed from expected 100 * MILLION_ETHER, will cause DoS of token distribution & permanent freezing of funds.

• Due to the fact that salt tokens dont exist in public before the airdrop & salt token distribution, it would be near impossible for an attacker to send non-existent salt tokens to this contract in order to DoS the distribution functionality and permanently freeze the salt token funds in there.

• However, there's likely no guarantee that the salt.balanceOf(address(this)) won't get slightly skewed anyway due to external factors, so it would be wise to implement guaranteed mitigation against this potential scenario...

• Therefore this is at least a QA and at most a medium severity issue.

• IF there was any salt tokens already in circulation before this distribution function was called, this would be a high/critical severity vulnerability...

L53:

require( salt.balanceOf(address(this)) == 100 * MILLION_ETHER, "SALT has already been sent from the contract" );

IMPACT:

• no impact, unless the salt.balanceOf(address(this)) was somehow skewed before token distribution, which is highly unlikely but not impossible. To make it impossible, the recommended "fix" should be implemented.

RECOMMENDATION:

• to change from == to >= to ensure the funds can never become stuck...
• always possible to transfer more salt tokens to this contract(if ever necessary), therefore the suggested modification below should suffice to permanently squash this vulnerability.

- require( salt.balanceOf(address(this)) == 100 * MILLION_ETHER, "SALT has already been sent from the contract" );
+ require( salt.balanceOf(address(this)) >= 100 * MILLION_ETHER, "SALT has already been sent from the contract" );

And then just modify/update the implemented logic where necessary to account for the above change, so that the intended logic behind the error message remains respected.

=======

10. ManagedWallet::receive() - L65: Contract intended to receive ETH via receive() but seems to be no way to ever get any ETH out again, which is problematic unless its deliberate intention of protocol team?

• .05 ether isnt a small amount, currently it's around $125, so when this is sent to this contract 10 times, it's already $1250 of ETH funds stuck forever in the contract...

• I checked and didn't see any methods/functions to withdraw the ETH funds again...

	// The confirmation wallet confirms or rejects wallet proposals by sending a specific amount of ETH to this contract
    receive() external payable
    	{
    	require( msg.sender == confirmationWallet, "Invalid sender" );

		// Confirm if .05 or more ether is sent and otherwise reject.
		// Done this way in case custodial wallets are used as the confirmationWallet - which sometimes won't allow for smart contract calls.
    	if ( msg.value >= .05 ether )
    		activeTimelock = block.timestamp + TIMELOCK_DURATION; // establish the timelock
    	else
			activeTimelock = type(uint256).max; // effectively never
        }

RECOMMENDATION:

• add withdraw method with access control so that admin/owner can withdraw the ETH if ever necessary.

via transfer():

    // Function to allow the owner to withdraw ETH from the contract
    function withdraw(uint256 amount) external onlyOwner {
        require(amount > 0, "Withdrawal amount must be greater than 0");
        require(address(this).balance >= amount, "Insufficient balance in the contract");

        // Transfer ETH to the owner
        payable(owner).transfer(amount);

        // Emit event
        emit Withdrawal(owner, amount);
    }

OR

via call():

function withdraw(uint256 amount) external onlyOwner {
    require(amount > 0, "Withdrawal amount must be greater than 0");
    require(address(this).balance >= amount, "Insufficient balance in the contract");

    // Call with explicit gas and value
    (bool success, ) = payable(owner).call{value: amount, gas: gasleft()}("");
    require(success, "Transfer failed");

    // Emit event
    emit Withdrawal(owner, amount);
}

=======

11. Proposals::proposeSendSALT() & DAO::_executeApproval() - lack of input validation for amount aka ballot.number1 parameter, can pass zero value and result in 0 salt safeTransferred.

function proposeSendSALT( address wallet, uint256 amount, string calldata description )

function _executeApproval( Ballot memory ballot )

=======