Despite the check in the constructor, weights can still be set to zero which would prevent user withdrawals

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-amm/UlyssesToken.sol#L88-L105 https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-4626/ERC4626MultiToken.sol#L202

Vulnerability details

Proof Of Concept

When initializing the weight for each token in ERC4626MultiToken, there's a check to prevent the weight for any token to be zero.

https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-4626/ERC4626MultiToken.sol#L52

However, it would still be possible to set the weight to zero on any token (weight[i]), by using the UlyssesToken.setWeights(). Currently this function doesn't have any check to prevent the weight being zero.

function setWeights(uint256[] memory _weights) external nonReentrant onlyOwner {
    if (_weights.length != assets.length) revert InvalidWeightsLength();

    weights = _weights;

    uint256 newTotalWeights;

    for (uint256 i = 0; i < assets.length; i++) {
        newTotalWeights += _weights[i];

        emit AssetRemoved(assets[i]);
        emit AssetAdded(assets[i], _weights[i]);
    }

    totalWeights = newTotalWeights;

    updateAssetBalances();
}

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-amm/UlyssesToken.sol#L88-L105

updateAssetsBalances() will still work, because the zero will be present on the numerator. However, the function withdraw() calls previewWithdraw() which calls convertToShares(). And convertToShares() will use weights[i] on the denominator.

https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-4626/ERC4626MultiToken.sol#L202

The mulDiv() from solady will revert when the denominator is zero.

https://github.com/Vectorized/solady/blob/main/src/utils/FixedPointMathLib.sol#L370

Impact

If the weight of any token is set to zero, user won't be able to withdraw. Setting the weight to zero could happen by accident, or in the worst case intentionally.

Although resetting the value back to a valid weight would fix the issue, until the fix is made, user assets' would be locked, and there's no guaranteed a fix would be made.

There's also the possibility of the owner keys getting compromised, in which an attacker could be the weight to zero to intentionally lock user assets.

Severity Rationale

likelihood is relatively small, because this would be subject to admin error, however impact is high since it would lock user assets, resulting in users loosing access to deposited assets and funds.

Tools Used

Manual review.

Recommended Mitigation Steps

Prevent setWeight of receiving the weight zero for any token.

diff --git a/UlyssesToken.sol.orig b/UlyssesToken.sol
--- a/UlyssesToken.sol.orig
+++ b/UlyssesToken.sol
@@ -93,6 +93,7 @@ contract UlyssesToken is ERC4626MultiToken, Ownable, IUlyssesToken {
         uint256 newTotalWeights;

         for (uint256 i = 0; i < assets.length; i++) {
+            require(_weights[i] > 0);
             newTotalWeights += _weights[i];

             emit AssetRemoved(assets[i]);

Assessed type

Invalid Validation

[c4-judge]

trust1995 marked the issue as unsatisfactory: Out of scope

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)

[c4-judge]

trust1995 marked the issue as grade-c