search

code-423n4 / 2024-01-salty-findings

11 stars 6 forks source link

Lack of logic to pause some critical functions in the Pools contract for after the Salty.IO DEX would be launched #596

Closed c4-bot-3 closed 9 months ago

c4-bot-3 commented 9 months ago

Lines of code

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

Vulnerability details

Impact

After the Salty.IO DEX would be launched, the owner (i.e. DAO) can not pause the critical functions in the Pools contract - even if some emergency or some serious negative situations would occur in the Pools contract.

Proof of Concept

Within the Pools contract, the exchangeIsLive would be defined to set to true when starting the exchange is approved by the bootstrapBallot like this: https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L43

    // Set to true when starting the exchange is approved by the bootstrapBallot
    bool public exchangeIsLive;

Within the Pools#startExchangeApproved(), true would be stored into the exchangeIsLive like this: https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L77

    function startExchangeApproved() external nonReentrant
        {
        require( msg.sender == address(exchangeConfig.initialDistribution().bootstrapBallot()), "Pools.startExchangeApproved can only be called from the BootstrapBallot contract" );
                 ...

        exchangeIsLive = true; ///<-------------------- @audit
        }

Within the Pools#addLiquidity(), whether or not the exchangeIsLive == true would be checked like this: https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L143

    // Add liquidity to the specified pool (must be a whitelisted pool)
    // Only callable from the CollateralAndLiquidity contract - so it can specify totalLiquidity with authority
    function addLiquidity( IERC20 tokenA, IERC20 tokenB, uint256 maxAmountA, uint256 maxAmountB, uint256 minLiquidityReceived, uint256 totalLiquidity ) external nonReentrant returns (uint256 addedAmountA, uint256 addedAmountB, uint256 addedLiquidity)
        {
        ...
        require( exchangeIsLive, "The exchange is not yet live" ); ///<----------------------- @audit

Once the Pools#startExchangeApproved() would be called by the BootstrapBallot contract and thereby true would be stored into the exchangeIsLive, the Salty.IO DEX would be launched.

On the other hand, there is no function to update the exchangeIsLive to false. Hence, within the Pools#addLiquidity(), a logic to pause (and unpause) the Pools#addLiquidity() is supposed to be implemented for after the Salty.IO DEX would be launched.

However, within the Pools#addLiquidity(), there is no way for the owner (i.e. DAO) to pause. This is problematic because there is no way for the owner (i.e. DAO) to pause the Pools#addLiquidity() - even if some emergency or some serious negative situations (i.e. the Pools contract would be compromised or manipulated by a malicious attack, etc) would occur after the Salty.IO DEX would be launched.

Also, the following functions in the Pools contract would be problematic because of the same reason with above.

Remarks: For example, when the Socket Protocol's was recently exploited, the Socket Protocol team paused their contract to minimize the extent of the damage caused like this:

The team had subsequently paused the contract to minimize the extent of the damage caused.

Tools Used

Recommended Mitigation Steps

Within the Pools contract, consider adding the whenPaused() / whenNotPaused() modifier of the Pausable module to the following functions, which is made by OpenZeppelin, in order for the owner (i.e. DAO) to be able to pause / unpause - if some emergency or some serious negative situations would happen after the Salty.IO DEX would be launched.

Assessed type

Access Control

c4-judge commented 9 months ago

Picodes marked the issue as unsatisfactory: Overinflated severity

Picodes commented 9 months ago

Invalidating as this is more a design choice than a security issue