QuickSwap and StellaSwap contest - trustindistrust's results

Low

Administrative function lacks address(0) check for changing sensitive address.

Location: 1

Description

AlgebraFactory.setOwner() function lacks a 0 address check. If an oversight or human error occurs during protocol management, the ownership of the contract would be permanently affected.

Suggested course of action

Add a require statement to check for the 0 address.

AlgebraPool.initialize() can be called by anyone

Location: 1

Description

AlgebraPool.initialize() configures state values for the contract, including the initial price. This function is not called during deployment of the Pool. It will normally be called using deployment scripts run by the protocol.

Unless the deployment of every pool is conducted via private RPC, it seems plausible that the call to initialize could be front-run. While the effects would be only transient, it would be better to mitigate the issue outright.

Suggested course of action

Apply the existing onlyFactoryOwner modifier to the function. The owner of the factory contract, whether EOA or multisig, could then make the call with whatever appropriate timing or means, without requiring a private RPC for the whole sequence.

Non-critical

Constructor lacks address(0) checks for setting sensitive addresses.

Location: 1

Description

AlgebraFactory's constructor lacks an address(0) check for the _poolDeployer address. While the other address argument could be fixed with an additional transaction, this address could not.

Suggested course of action

Add a require statement to check for the 0 address.

Consider renaming setIncentive

Location: 1

Description

The interface documentation for setIncentive() indicates that it is setting "The address of a virtual pool associated with the incentive." The code does this. However, the name of the function implies that it is directly setting an 'incentive' value, and not the address of a contract.

Suggested course of action

Consider renaming the function to setIncentivePool or setIncentivePoolAddress.

AlgebraPool.getInnerCumulatives contains fragile branching code that may cause errors in future edits

Location: 1

Description

getInnerCumulatives() contains branching code based on the comparison of the currentTick value with the bottomTick and topTick values.

The third case where currentTick is greater than topTick is implicit in this version of the code; the final return is only encountered if the other two tests are false.

While there is nothing wrong with the execution of this code per se, it is fragile. With multiple return statements and lack of explicit if {} else if {} else {} structure, future developers might cause control flow issues if changes to this function are made.

Further, the function contains named output values. Generally, the form when using named return values is to assign them as needed and then allow execution to leave the scope, rather than using an explicit return.

Suggested course of action

Modify the function to use explicit if {} else if {} else {}.

Consider either a) removing the named return values and using return statements b) keeping the named values but removing the explicit returns, or c) condensing the multiple returns into one return.

For example, AlgebraPool._getAmountsForLiquidity implements this similar set of checks in a way that's robust and idiomatic.

Using named return values without assignment

Locations: 1 2 3 4 5 6 7 <- generates compiler warning if addressed as suggested

Description

Solidity allows for naming return values from functions. This can ease code comprehension, and cuts down on explicit return statements.

However, the indicated lines combine named return values with explicit return statements. This makes function signatures needlessly verbose, as the returns() can be shorted considerably in length.

For example, the proper style is seen in DataStorageOperator.getSingleTimepoint().

Suggested course of action

Remove unneeded named return values if they aren't necessary.

DataStorage.getAverages() contains unnecessary storage read

Location: 1

Description

DataStorage.getAverages() contains a check to handle the case that the next index in an array of data has intentionally overflowed to 0.

uint16 oldestIndex;
Timepoint storage oldest = self[0];
uint16 nextIndex = index + 1; // considering overflow

if (self[nextIndex].initialized) {
    oldest = self[nextIndex];
    oldestIndex = nextIndex;
}

If the if case is true, then the array index has overflowed and thus oldest already points to the correct struct in the array. Thus the storage read is unnecessary, as is the assignment.

Suggested course of action

The warden ran the entire test suite and no failed tests were detected. As a result, it seems safe to remove the unneeded assignment oldest = self[nextIndex];

Use multiple require statements instead of evaluating multiple conditions with &&

Locations: 1 2 3 4 5 6

Description

Using two (or more) require statements instead of a single require with an internal logical AND check is slightly cheaper and eases understanding of the checks.

Suggested course of action

Break up checks.

Note that the examples require enhanced deployment costs due to potentially repeating the same revert string. It could be beneficial to make the strings unique to enhance error reporting, or only return a string on the final check.

Replace > 0 checks with != 0 in require statements

Locations: 1 2 3 4 5 6

Description

When testing unsigned integers for non-zero values, it is slightly cheaper to use != 0 than to use > 0.

Suggested course of action

Implement the described change.

Remove computeAddress function and replace it's call with inlined computation, saving 48 gas per deploy.

Location: 1

Description

The function AlgebraFactory.createPool() makes a call to AlgebraFactory.computeAddress(). This function is only referenced once in the project, thus inflating the deployment cost of the contract.

Suggested course of action

Re-write createPool to inline the computation of the address, similar to the following:

    function createPool(address tokenA, address tokenB) external override returns (address pool) {
        require(tokenA != tokenB);
        (address token0, address token1) = tokenA < tokenB ? (tokenA, tokenB) : (tokenB, tokenA);
        require(token0 != address(0));
        require(poolByPair[token0][token1] == address(0));

        IDataStorageOperator dataStorage = 
            new DataStorageOperator(address(uint256(keccak256(abi.encodePacked( hex'ff', poolDeployer, keccak256(abi.encode(token0, token1)), POOL_INIT_CODE_HASH )))));

        dataStorage.changeFeeConfiguration(baseFeeConfiguration);

        pool = IAlgebraPoolDeployer(poolDeployer).deploy(address(dataStorage), address(this), token0, token1);

        poolByPair[token0][token1] = pool; // to avoid future addresses comparing we are populating the mapping twice
        poolByPair[token1][token0] = pool;
        emit Pool(token0, token1, pool);
    }

Additionally, it would be best to move POOL_INIT_CODE_HASH to the top of the contract where the other global values are, or move it into Constants.sol.