Valantis Security Review Report

In the Validly.sol contract the error Validly__priceOutOfRange on line 29, is declared but never used.

error Validly__priceOutOfRange();

If the error is redundant and there is no missing logic where it can be used, it should be removed.

In mentioned contracts, there are constant and immutable variables that are declared public. However, setting constants and immutables to private will save deployment gas. This is because the compiler won't have to create non-payable getter functions for deployment calldata, won't need to store the bytes of the value outside of where it's used, and won't add another entry to the method ID table. If necessary, the values can still be read from the verified contract source code.

uint256 public constant MINIMUM_LIQUIDITY = 1000;
 
ISovereignPool public immutable pool;
 
bool public immutable isStable;
 
uint256 public immutable decimals0;
 
uint256 public immutable decimals1;
 
IProtocolFactory public immutable protocolFactory;
 
uint256 public immutable feeBips;

The mentioned variables should be marked as private instead of public.

The ValidlyFactory.createPair() function deploys a new instance of the Sovereign Pool, passing address(this) (the ValidlyFactory itself) to the constructor as the poolManager. This gives the ValidlyFactory control over pool manager-restricted functions, such as:

• setPoolManager()
• setPoolManagerFeeBips()
• setSovereignOracle()
• setSwapFeeModule()
• claimPoolManagerFees() However, the ValidlyFactory contract currently only implements claimFees() and setPoolManagerFeeBips() to interact with the pool's claimPoolManagerFees() and setPoolManagerFeeBips(). It lacks functions to interact with the other critical pool configuration functions. This omission may prevent the pool from updating settings, such as changing the oracle, when needed.

SovereignPoolConstructorArgs memory args = SovereignPoolConstructorArgs(
    _token0,
    _token1,
    address(protocolFactory),
    address(this),
    address(0),
    address(0),
    false,
    false,
    0,
    0,
    feeBips
);

Consider adding more functions for the ValidlyFactory contract to interact with the SovereignPool contract.

In the Validly.deposit() function, there is a check to revert the transaction if the _recipient address is address(0). However, this check is unnecessary, as the internal _mint() function in the ERC20 contract already performs the same check, ensuring that the recipient address is not address(0).

abstract contract ERC20 is Context, IERC20, IERC20Metadata, IERC20Errors {
 
    ...
 
    function _mint(address account, uint256 value) internal {
 
        if (account == address(0)) {
 
            revert ERC20InvalidReceiver(address(0));
 
        }
 
        _update(address(0), account, value);
 
    }
 
    ...
 
}

if (_recipient == address(0)) {
 
    revert Validly__deposit_invalidRecipient();
 
}

Consider removing the address(0) check for the _recipient address.

function deposit(
    uint256 _amount0,
    uint256 _amount1,
    uint256 _minShares,
    uint256 _deadline,
    address _recipient,
    bytes calldata _verificationContext
)
    external
    override
    ensureDeadline(_deadline)
    nonReentrant
    returns (uint256 shares, uint256 amount0, uint256 amount1)
{
-    if (_recipient == address(0)) {
-        revert Validly__deposit_invalidRecipient();
-    }

The function Validly._get_y_stableInvariant loops from 0 to 254, resulting in a total of 255 iterations. However, the comment states, "Did not converge in 256 fixed point iterations," which does not match the actual 255 iterations.

function _get_y_stableInvariant(uint256 x0, uint256 invariant, uint256 y)
private pure returns (uint256) {
 
    for (uint256 i = 0; i < 255; i++) {
 
        uint256 y_prev = y;
 
        uint256 k = _f(x0, y);
 
        if (k < invariant) {
 
            uint256 dy = ((invariant - k) * 1e18) / _d(x0, y);
 
            y = y + dy;
 
        } else {
 
            uint256 dy = ((k - invariant) * 1e18) / _d(x0, y);
 
            y = y - dy;
 
        }
 
        if (y > y_prev) {
 
            if (y - y_prev <= 1) {
 
                return y;
 
            }
 
        } else {
 
            if (y_prev - y <= 1) {
 
                return y;
 
            }
 
        }
 
    }
 
    // Did not converge in 256 fixed point iterations,
 
    // revert for safety
 
    revert Validly___get_y_notConverged();
 
}

Consider fixing the comment to:

- Did not converge in 256 fixed point iterations
+ Did not converge in 255 fixed point iterations