Pool Manual Review Findings

POO-01M: Improper State Upgrade Flow

Description:

The contract is built in a way that permits its core mathematical values to be configured, namely its K, N, C1 and XThreshold values. However, updates are performed atomically whilst the values of C1 and XThreshold depend on the values of K and N directly.

Example:

Copy
292/**
293 * @notice Changes the pools slippage param K. Can only be set by the contract owner.
294 * @param k_ new pool's slippage param K
295 */
296function setSlippageParamK(uint256 k_) external onlyOwner {
297    require(k_ <= ETH_UNIT); // k should not be set bigger than 1
298    uint256 oldK = _slippageParamK;
299    _slippageParamK = k_;
300    emit SlippageParamKUpdated(oldK, k_);
301}
302
303/**
304 * @notice Changes the pools slippage param N. Can only be set by the contract owner.
305 * @param n_ new pool's slippage param N
306 */
307function setSlippageParamN(uint256 n_) external onlyOwner {
308    uint256 oldN = _slippageParamN;
309    _slippageParamN = n_;
310    emit SlippageParamNUpdated(oldN, n_);
311}
312
313/**
314 * @notice Changes the pools slippage param C1. Can only be set by the contract owner.
315 * @param c1_ new pool's slippage param C1
316 */
317function setC1(uint256 c1_) external onlyOwner {
318    uint256 oldC1 = _c1;
319    _c1 = c1_;
320    emit SlippageParamC1Updated(oldC1, c1_);
321}
322
323/**
324 * @notice Changes the pools slippage param xThreshold. Can only be set by the contract owner.
325 * @param xThreshold_ new pool's slippage param xThreshold
326 */
327function setXThreshold(uint256 xThreshold_) external onlyOwner {
328    uint256 oldXThreshold = _xThreshold;
329    _xThreshold = xThreshold_;
330    emit SlippageParamXThresholdUpdated(oldXThreshold, xThreshold_);
331}

Recommendation:

We advise the configuration flow of the contract to be revised as a transaction in between a configuration adjustment can have devastating consequences to the protocol. As a solution, we advise one function to be invoked to set all these values. Additionally, we advise another level of sanitization to be applied on the N parameter as it should always be a positive number given that a value of 0 will cause a positive infinity value for C1.

Alleviation:

The slippage parameters are now always set simultaneously and an additional sanitisation for the value of n_ has been introduced according to our recommendation thereby alleviating this exhibit in full.

POO-02M: Inexplicable Repeat Invocation Capability

Description:

The setWETH and setWETHForwarder adjust a sensitive system component that should not be changed under code-complete circumstances.

Example:

Copy
284/**
285 * @notice Changes the pools WETHForwarder. Can only be set by the contract owner.
286 * @param wethForwarder new pool's WETHForwarder address
287 */
288function setWETHForwarder(address payable wethForwarder) external onlyOwner {
289    _wethForwarder = WETHForwarder(wethForwarder);
290}

Recommendation:

We advise the capability to be omitted from the codebase as the forwarder and WETH representation are integral system components whose compromisation can cause all WETH based trades to be hijacked.

Alleviation:

The function is no longer part of the codebase rendering this exhibit irrelevant.

POO-03M: Potentially Improper Aggregation Evaluation

Description:

Should the values of the aggregateAccount be unset, assets will be considered equivalent as the default unset values will both be equal to the zero address.

Example:

Copy
752// assets need to be in the same aggregate in order to allow for withdrawing other assets
753require(wantedAsset.aggregateAccount() == initialAsset.aggregateAccount(), 'DIFF_AGG_ACC');

Recommendation:

We advise another level of validation to be performed whereby the aggregateAccount of either asset is evaluated to be non-zero to ensure that the assets indeed belong to an aggregate account.

Alleviation:

Upon discussion with the Platypus team, the aggregateAccount of an asset cannot be unset and as such this exhibit can be considered null.