StrategyV5Agent Manual Review Findings

SVA-01M: Discrepant Allowance Maintenance

Type	Severity	Location
Logical Fault		StrategyV5Agent.sol:L53, L143-L144

Description:

The StrategyV5Agent::setSwapperAllowance function will ensure that a new swapper will be properly authorized to swap the reward tokens of the contract, however, the StrategyV5Agent::setRewardTokens function fails to do this thereby causing newly configured reward tokens to lack the necessary approval to be utilized.

Impact:

Configuration of new reward tokens will cause them to be inoperable by the swapper and would require multiple actions for the swapper to be properly approved for them.

Example:

Copy
134/**
135 * @notice Sets the reward tokens
136 * @param _rewardTokens array of reward tokens
137 */
138function setRewardTokens(
139    address[] calldata _rewardTokens
140) public onlyManager {
141    if (_rewardTokens.length > 8) revert Unauthorized();
142    for (uint8 i = 0; i < _rewardTokens.length; i++) {
143        rewardTokens[i] = _rewardTokens[i];
144        rewardTokenIndex[_rewardTokens[i]] = i+1;
145    }
146    rewardLength = uint8(_rewardTokens.length);
147}

Recommendation:

We advise the StrategyV5Agent::setRewardTokens function to properly set allowances, ensuring the swapper can utilize them as necessary.

Alleviation (59b75fbee1d8f3dee807c928f18be41c58b904e1):

The StrategyV5Agent::_setRewardTokens function, representing an internalization of the original StrategyV5Agent::setRewardTokens logic, was updated to properly supply approvals for the newly configured reward tokens rendering this exhibit alleviated.

SVA-02M: Improper No-Op Logic Statement

Type	Severity	Location
Language Specific		StrategyV5Agent.sol:L49

Description:

The referenced statement will not result in any functional change to the code as it will evaluate a ternary operator and not utilize the result.

Impact:

The code is meant to treat a 0 value allowance as the maximum but presently ignores it.

Example:

Copy
41/**
42 * @notice Sets the swapper allowance
43 * @param _amount Amount of allowance to set
44 */
45function setSwapperAllowance(uint256 _amount) public onlyAdmin {
46    address swapperAddress = address(swapper);
47    // we keep the possibility to set allowance to 0 in case of a change of swapper
48    // default is to approve MAX_UINT256
49    _amount != 0 ? _amount : MAX_UINT256;
50
51    for (uint256 i = 0; i < rewardLength; i++) {
52        if (rewardTokens[i] == address(0)) break;
53        IERC20Metadata(rewardTokens[i]).approve(swapperAddress, _amount);
54    }
55    for (uint256 i = 0; i < inputLength; i++) {
56        if (address(inputs[i]) == address(0)) break;
57        inputs[i].approve(swapperAddress, _amount);
58    }
59    asset.approve(swapperAddress, _amount);
60}

Recommendation:

We advise the behaviour of the StrategyV5Agent::setSwapperAllowance to be validated and the ternary operator to either be removed or incorporated within it.

Alleviation (59b75fbee1d8f3dee807c928f18be41c58b904e1):

The code was updated to properly utilize the result of the ternary statement in an assignment to the _amount variable, addressing this exhibit.

SVA-03M: Inexistent Erasure of Previous Approvals

Type	Severity	Location
Logical Fault		StrategyV5Agent.sol:L111, L126, L143

Description:

The various functions of the StrategyV5Agent contract that permit the inputs, reward tokens, and underlying asset to be adjusted do not erase the previously present approval to the swapper, permitting lingering approvals to remain in the code.

Impact:

Approvals that are a result of replaced assets will remain to the swapper even if it is replaced, signifying a potential flaw in the system that can affect funds.

Example:

Copy
114/**
115 * @notice Sets the input tokens (strategy internals), make sure to liquidate() them first
116 * @param _inputs array of input tokens
117 * @param _weights array of input weights
118 */
119function setInputs(
120    address[] calldata _inputs,
121    uint16[] calldata _weights
122) public onlyAdmin {
123    if (_inputs.length > 8) revert Unauthorized();
124    address swapperAddress = address(swapper);
125    for (uint8 i = 0; i < _inputs.length; i++) {
126        inputs[i] = IERC20Metadata(_inputs[i]);
127        inputDecimals[i] = inputs[i].decimals();
128        inputWeights[i] = _weights[i];
129        inputs[i].approve(swapperAddress, MAX_UINT256);
130    }
131    inputLength = uint8(_inputs.length);
132}
133
134/**
135 * @notice Sets the reward tokens
136 * @param _rewardTokens array of reward tokens
137 */
138function setRewardTokens(
139    address[] calldata _rewardTokens
140) public onlyManager {
141    if (_rewardTokens.length > 8) revert Unauthorized();
142    for (uint8 i = 0; i < _rewardTokens.length; i++) {
143        rewardTokens[i] = _rewardTokens[i];
144        rewardTokenIndex[_rewardTokens[i]] = i+1;
145    }
146    rewardLength = uint8(_rewardTokens.length);
147}

Recommendation:

We advise the code to properly erase any approval that previously existed, ensuring that no lingering approvals to potentially unauthorized swappers remain.

To note, the input and reward token configuration functions will also need to iterate up to the end of the inputs / rewardTokens array respectively to ensure a shrink of the array will also cause approvals to be erased.

Alleviation (59b75fbee1d8f3dee807c928f18be41c58b904e1):

The StrategyV5Agent::_setInputs and StrategyV5Agent::_setRewardTokens functions, both representing internalized implementations of their original public un-prefixed counterparts, have been updated to erase any previously existing approval when tokens are updated effectively alleviating this exhibit in full.

SVA-04M: Inexistent Protection Against Re-Initialization

Type	Severity	Location
Logical Fault		StrategyV5Agent.sol:L31-L39

Description:

The StrategyV5Agent::init function does not prevent against re-initialization, permitting the asset to be updated without the proper flow defined in StrategyV5Agent::updateAsset.

Impact:

A severity of minor has been assigned as the function is privileged, however, its impact is significant as the asset's immediate adjustment without a proper migration can cause the strategy to misbehave greatly.

Example:

Copy
27/**
28 * @notice Initialize the strategy
29 * @param _params StrategyBaseParams struct containing strategy parameters
30 */
31function init(StrategyBaseParams calldata _params) public onlyAdmin {
32    // setInputs(_params.inputs, _params.inputWeights);
33    setRewardTokens(_params.rewardTokens);
34    asset = IERC20Metadata(_params.coreAddresses.asset);
35    assetDecimals = asset.decimals();
36    weiPerAsset = 10**assetDecimals;
37    updateSwapper(_params.coreAddresses.swapper);
38    As4626.init(_params.erc20Metadata, _params.coreAddresses, _params.fees);
39}

Recommendation:

We advise the function to prevent re-invocation via a dedicated variable, ensuring the contract cannot be re-initialized.

Alleviation (59b75fbee1):

The Astrolab DAO team specified that they intend to supply an initialized public bool that will prevent re-initialization, however, no such change has been incorporated in the codebase yet.

As such, we consider this exhibit open in the codebase's current state.

Alleviation (efbeab6478):

Initialization protection has been introduced to the As4626::_init function, rendering this exhibit alleviated as a result.

SVA-05M: Insecure Approval Operations

Type	Severity	Location
Logical Fault		StrategyV5Agent.sol:L53, L57, L111, L129

Description:

The referenced approval operations may fail if the underlying token prevents approval reconfigurations when a non-zero approval exists.

Impact:

Presently, a reconfiguration of the inputs of the StrategyV5Agent may fail due to one of the tokens being present in both the old and new inputs and thus causing the approval to fail.

Example:

Copy
114/**
115 * @notice Sets the input tokens (strategy internals), make sure to liquidate() them first
116 * @param _inputs array of input tokens
117 * @param _weights array of input weights
118 */
119function setInputs(
120    address[] calldata _inputs,
121    uint16[] calldata _weights
122) public onlyAdmin {
123    if (_inputs.length > 8) revert Unauthorized();
124    address swapperAddress = address(swapper);
125    for (uint8 i = 0; i < _inputs.length; i++) {
126        inputs[i] = IERC20Metadata(_inputs[i]);
127        inputDecimals[i] = inputs[i].decimals();
128        inputWeights[i] = _weights[i];
129        inputs[i].approve(swapperAddress, MAX_UINT256);
130    }
131    inputLength = uint8(_inputs.length);
132}

Recommendation:

We advise usage of OpenZeppelin's SafeERC20 library and specifically its SafeERC20::forceApprove function, ensuring that approval overwrites are correctly performed.

Alleviation (59b75fbee1d8f3dee807c928f18be41c58b904e1):

All IERC20::approve instances have been replaced by OpenZeppelin's SafeERC20::forceApprove function, ensuring that they will be performed properly regardless of the underlying allowance's state thereby alleviating this exhibit.