Skip to content

Conversation

@gnarvaja
Copy link
Contributor

From QS Audit Report:

The following is a list of places that can potentially benefit from stricter input validation:

  1. SwapStableInvestStrategy.sol:
    a. the decimal asset_ of the constructor() function should be less than 18 .
    b. the decimal investAsset_ of the constructor() function should be less than 18 .
    c. the swapConfig.maxSlippage of the _setSwapConfig should be less than WAD .

  2. AccessManagedMSV.sol:
    a. the asset_ of initialize() should not be zero address.

  3. CompoundV3InvestStrategy.sol:
    a. the inputs of constructor() should not be zero addresses.

Validations 1.a and 1.b implemented. Also, I added a validation to check that asset_ != investAsset_.

I decided not to implement the maxSlippage validation, because I don't think is needed and also I don't wan't to break the abstraction of the SwapLibrary with the swap config. In any case, the setSwapConfig must be used wisely, adding a <WAD validation doesn't add much protection.

Regarding point 2, I decided not to apply the validation, since the parameter is sent to OZ's ERC4626 contract and they decided not to add that validation. Also, other validations of the initialization will forbid deploying a vault with asset = address(0).

Regarding point 3, I implemented a validation around the rewardManager parameter. If cToken_ parameter is zero, the constructor will fail anyway.

From QS Audit Report:

The following is a list of places that can potentially benefit from stricter input validation:
1. SwapStableInvestStrategy.sol:
 a. the decimal asset_ of the constructor() function should be less than 18 .
 b. the decimal investAsset_ of the constructor() function should be less than 18 .
 c. the swapConfig.maxSlippage of the _setSwapConfig should be less than WAD .

2. AccessManagedMSV.sol:
 a. the asset_ of initialize() should not be zero address.

3. CompoundV3InvestStrategy.sol:
 a. the inputs of constructor() should not be zero addresses.

Validations 1.a and 1.b implemented. Also, I added a validation to check
that `asset_ != investAsset_`.

I decided not to implement the maxSlippage validation, because I don't
think is needed and also I don't wan't to break the abstraction of the
SwapLibrary with the swap config. In any case, the setSwapConfig must be
used wisely, adding a `<WAD` validation doesn't add much protection.

Regarding point #2, I decided not to apply the validation, since the
parameter is sent to OZ's ERC4626 contract and they decided not to add
that validation. Also, other validations of the initialization will
forbid deploying a vault with asset = address(0).

Regarding point #3, I implemented a validation around the rewardManager
parameter. If `cToken_` parameter is zero, the constructor will fail
anyway.
@gnarvaja gnarvaja merged commit 61869e5 into main Mar 11, 2025
2 checks passed
@gnarvaja gnarvaja deleted the fix-qs-s1 branch March 11, 2025 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants