Pool Manual Review Findings

PLO-01M: Arbitrary Unique ID

Type	Severity	Location
Code Style		Pool.sol:L361

Description:

The pricingModelId of 99 uses a specialized calculation mechanism that is inexplicable.

Example:

Copy
361if (pricingModelId != 99) {
362    if (isExactIn) {
363        buyerTokenAmount = getAmountOut(
364            sellerTokenAmount,
365            _order.sellerToken,
366            _order.buyerToken
367        );
368    } else {
369        sellerTokenAmount = getAmountIn(
370            buyerTokenAmount,
371            _order.sellerToken,
372            _order.buyerToken
373        );
374    }
375} else {
376    require(
377        _order.sellerTokenAmount > 0 && _order.buyerTokenAmount > 0,
378        "Non-zero amount required"
379    );
380
381    if (sellerTokenAmount > 0) {
382        buyerTokenAmount = FullMath.mulDiv(
383            sellerTokenAmount,
384            _order.buyerTokenAmount,
385            _order.sellerTokenAmount
386        );
387    } else if (buyerTokenAmount > 0) {
388        sellerTokenAmount = FullMath.mulDiv(
389            buyerTokenAmount,
390            _order.sellerTokenAmount,
391            _order.buyerTokenAmount
392        );
393    }
394}

Recommendation:

We advise the documentation of the codebase to be enhanced and the literal to be relocated to a contract-level constant, optimizing the legibility of the codebase. As an additional step, we advise the conditional mechanism to be omitted entirely and the pricing model employed in the else branch to simply be implemented in the pricing registry.

Alleviation:

The 99 value literal was relocated to a contract-level constant declaration labelled FIXED_PRICE_MODEL_ID. This indicates that the exchange ratio is dictated by the off-chain service of Native rather than on-chain data. As the payload is validated as signed in the swap function that invokes calculateTokenAmount, we consider this exhibit alleviated and behaving as expected.

PLO-02M: Inexplicable Loop Iteration

Type	Severity	Location
Logical Fault		Pool.sol:L235, L237-L250

Description:

The executeUpdatePairs function permits only one pair to be added due to the require check mandating _fees.length to be less-than-or-equal-to 1.

Example:

Copy
234function executeUpdatePairs(uint256[] memory _fees, address[] memory _tokenAs, address[] memory _tokenBs, uint256[] memory _pricingModelIds) private {
235    require(_fees.length <= 1, "Each pool only supports 1 pair.");
236    require(isPairAdded == false || pairExist(_tokenAs[0], _tokenBs[0]), "Please remove existing pair before adding a new pair.");
237    for (uint i = 0; i < _fees.length; i++) {
238        require(_tokenAs[i] != _tokenBs[i], "Identical addresses");
239        (address token0, address token1) = _tokenAs[i] < _tokenBs[i]
240            ? (_tokenAs[i], _tokenBs[i])
241            : (_tokenBs[i], _tokenAs[i]);
242        require(token0 != address(0), "Zero address in pair");                        
243        if (pairExist(token0, token1) == false) {
244            tokenAs.push(token0);
245            tokenBs.push(token1);
246            isPairAdded = true;
247            pairCount++;
248        }            
249        pairs[token0][token1] = Pair(_fees[i], true, _pricingModelIds[i]);
250    }
251    
252}

Recommendation:

We advise the code to instead be revamped to implement a single pool instead, adjusting all relevant code that utilizes array based entries.

Alleviation:

The Native team evaluated this exhibit but opted not to apply a remediation for it as they consider it inconsequential.

PLO-03M: Inexistent Limitation of Fee

Type	Severity	Location
Input Sanitization		Pool.sol:L249

Description:

The _fees[i] value that is assigned to a particular pair is not sanitized in the executeUpdatePairs function.

Impact:

Mistyped fees can cause pairs to become inoperable or to charge abnormally high fees.

Example:

Copy
234function executeUpdatePairs(uint256[] memory _fees, address[] memory _tokenAs, address[] memory _tokenBs, uint256[] memory _pricingModelIds) private {
235    require(_fees.length <= 1, "Each pool only supports 1 pair.");
236    require(isPairAdded == false || pairExist(_tokenAs[0], _tokenBs[0]), "Please remove existing pair before adding a new pair.");
237    for (uint i = 0; i < _fees.length; i++) {
238        require(_tokenAs[i] != _tokenBs[i], "Identical addresses");
239        (address token0, address token1) = _tokenAs[i] < _tokenBs[i]
240            ? (_tokenAs[i], _tokenBs[i])
241            : (_tokenBs[i], _tokenAs[i]);
242        require(token0 != address(0), "Zero address in pair");                        
243        if (pairExist(token0, token1) == false) {
244            tokenAs.push(token0);
245            tokenBs.push(token1);
246            isPairAdded = true;
247            pairCount++;
248        }            
249        pairs[token0][token1] = Pair(_fees[i], true, _pricingModelIds[i]);
250    }
251    
252}

Recommendation:

We advise it to be adequately sanitized as representing a valid percentage based on the IPricer implementations of the repository (UniswapV2Pricer & ConstantSumPricer).

Alleviation:

The fee is now mandated to be within a valid range, however, this is done inefficiently as it evaluates that _fees[i] >= 0 when _fees[i] represents an unsigned integer which will always be non-negative. In any case, we consider this exhibit alleviated as the upper bound is properly applied.

PLO-04M: Inexplicable Payable Function Attribute

Type	Severity	Location
Language Specific		Pool.sol:L148

Description:

The swap function is attributed as payable, however, it does not handle native funds causing them to be permanently locked.

Impact:

Any funds transmitted to the Pool contract as part of a swap call will be permanently locked in the contract.

Example:

Copy
148) external payable override nonReentrant whenNotPaused onlyRouter noDelegateCall returns (int256, int256) {

Recommendation:

We advise the function to have its payable attribute omitted as it serves no purpose.

Alleviation:

The payable function attribute has been safely omitted as advised.

PLO-05M: Unsafe Casting Operations

Type	Severity	Location
Language Specific		Pool.sol:L480, L481

Description:

The referenced statements perform an unsafe casting operation from a uint256 data type to an int256 data type.

Impact:

As the casting overflow will benefit the user due to its presence in the executeSwapToTreasury function, a carefully crafted order payload can be exploited to extract a set of funds from the treasury whilst transmitting close to none.

Example:

Copy
480int256 outputSellerTokenAmount = int256(sellerTokenAmount);
481int256 outputBuyerTokenAmount = -1 * int256(buyerTokenAmount);

Recommendation:

We advise the casting operations to be performed safely as they can presently overflow and become corrupted.

Alleviation:

The limitations on the input variables have been properly applied, however, they inefficiently use the value of type(uint256).max / 2 as a comparator rather than the value of type(int256).max.