Omniscia Steer Protocol Audit

FeeManager Manual Review Findings

FeeManager Manual Review Findings

FMR-01M: Discrepant Sanitization of Total Vault Fees

Description:

Each entry written to the vaultTotalFees is sanitized differently or even remains unsanitized depending on the function invoked.

Impact:

The vaultTotalFees does not abide by a specific range of values, and is even unsanitized in the case of the FeeManager::setDefaultFeeAndWithdrawalPermission function which we consider invalid.

Example:

contracts/FeeManager.sol
204function setTotalFees(
205    address vault,
206    uint256 newTotalFees
207) external onlyOwner {
208    require(newTotalFees < 5000, "Total fee must be less than 5000");
209    vaultTotalFees[vault] = newTotalFees;
210}

Recommendation:

We advise validation of the vaultTotalFees entry to be streamlined across the contract, ensuring a single limitation is imposed on it across all functions that overwrite it.

Alleviation (6513a21a002d422e298719b22f73a4559dfd4663):

Total fee validation across the contract has been streamlined to validate an upper bound of 10000, alleviating this exhibit.

FMR-02M: Inexistent Prevention of Duplicate Fee Identifiers

Description:

The FeeManager::setFeeAndWithdrawalPermission and FeeManager::setDefaultFeeAndWithdrawalPermission functions do not prevent duplicate identifiers from being specified, a trait that will result in a discrepancy for the withdrawalPermissions entry.

Impact:

Duplicate fee identifiers in one of the two aforementioned configuration calls will result in the final withdrawer of the duplicate entries to remain in the withdrawalPermissions entry, a trait we consider invalid.

Example:

contracts/FeeManager.sol
58function setFeeAndWithdrawalPermission(
59    address vault,
60    string[] memory feeIdentifier,
61    uint256[] memory feeValue,
62    address[] memory withdrawer
63) external onlyOwner {
64    require(
65        feeIdentifier.length == feeValue.length &&
66            feeValue.length == withdrawer.length,
67        "Input arrays must have the same length"
68    );
69    uint256 totalFeeValue;
70    if (vaultFees[vault].length > 0) delete vaultFees[vault]; //Always make to pass all the tiers again when adding more tiers
71    for (uint256 i = 0; i < feeIdentifier.length; i++) {
72        require(
73            withdrawer[i] != address(0),
74            "Withdrawer address cannot be zero"
75        );
76        vaultFees[vault].push(Fee(feeIdentifier[i], feeValue[i]));
77        withdrawalPermissions[vault][feeIdentifier[i]] = withdrawer[i];
78        totalFeeValue += feeValue[i];
79    }
80    require(
81        totalFeeValue == 10000,
82        "Total fee value must be exactly 10000"
83    );
84    emit FeeUpdated(vault, feeIdentifier, feeValue, withdrawer);
85}
86

87function setDefaultFeeAndWithdrawalPermission(
88    address vault,
89    uint256 totalVaultFees,
90    string[] memory feeIdentifier,
91    uint256[] memory feeValue,
92    address[] memory withdrawer
93) external {
94    require(vaultRegistry == msg.sender, "!VaultRegistry");
95    require(
96        feeIdentifier.length == feeValue.length &&
97            feeValue.length == withdrawer.length,
98        "Input arrays must have the same length"
99    );
100    uint256 totalFeeValue;
101    for (uint256 i = 0; i < feeIdentifier.length; i++) {
102        require(
103            withdrawer[i] != address(0),
104            "Withdrawer address cannot be zero"
105        );
106        vaultFees[vault].push(Fee(feeIdentifier[i], feeValue[i]));
107        withdrawalPermissions[vault][feeIdentifier[i]] = withdrawer[i];
108        totalFeeValue += feeValue[i];
109    }
110    require(
111        totalFeeValue == 10000,
112        "Total fee value must be exactly 10000"
113    );
114    vaultTotalFees[vault] = totalVaultFees;
115    emit FeeUpdated(vault, feeIdentifier, feeValue, withdrawer);
116}

Recommendation:

We advise the code to ensure that each feeIdentifier is unique.

This can be achieved by ensuring that their input order is strictly ascending by evaluating each identifier's keccak256 hash, or by using a new withdrawalPermissions subset on each re-configuration and ensuring each entry being written to was previously zero.

Alleviation (6513a21a00):

A FeeManager::areAllUnique function has been introduced to the contract that will ensure all fee identifiers are unique by comparing them in looping iterations.

The validation mechanism can be made significantly more efficient by ensuring that the keccak256 results of the strings are in strictly ascending manner, permitting uniqueness validation in just a single iteration.

Alleviation (dc34c6a066):

The Steer Protocol team evaluated our recommendation, and believe that the operational burden imposed by off-chain keccak256 based ordering does not outweigh the gas benefits attached to it.

As the original vulnerability described has been alleviated, we consider this exhibit adequately addressed.

FMR-03M: Out-of-Bound Index Access Failures

Description:

The FeeManager::withdrawMultipleFees function will fail to execute if any of the amount prefixed arrays have a length of 0, as the code will attempt to write to them in such a case.

Impact:

The functionality of the code that automatically calculates the accrued fees for a particular fee identifier is inaccessible due to unconditional out-of-bound array assignments.

Example:

contracts/FeeManager.sol
247function withdrawMultipleFees(
248    address[] memory vaults,
249    string[] memory feeIdentifiers,
250    uint256[] memory amount0,
251    uint256[] memory amount1
252) public {
253    require(
254        vaults.length == feeIdentifiers.length,
255        "Vaults and feeIdentifiers length mismatch"
256    );
257    address[] memory to;
258    if (amount0.length > 0 || amount1.length > 0) {
259        for (uint256 i = 0; i < vaults.length; i++) {
260            to[i] = withdrawalPermissions[vaults[i]][feeIdentifiers[i]];
261            IMultiPositionManager(vaults[i]).collectFees(
262                feeIdentifiers[i],
263                amount0[i],
264                amount1[i]
265            );
266        }
267    } else {
268        for (uint256 i = 0; i < vaults.length; i++) {
269            to[i] = withdrawalPermissions[vaults[i]][feeIdentifiers[i]];
270            amount0[i] = IMultiPositionManager(vaults[i]).accruedFees0(
271                feeIdentifiers[i]
272            );
273            amount1[i] = IMultiPositionManager(vaults[i]).accruedFees1(
274                feeIdentifiers[i]
275            );
276            IMultiPositionManager(vaults[i]).collectFees(
277                feeIdentifiers[i],
278                amount0[i],
279                amount1[i]
280            );
281        }
282    }
283    emit FeesWithdrawn(
284        msg.sender,
285        vaults,
286        feeIdentifiers,
287        to,
288        amount0,
289        amount1
290    );
291}

Recommendation:

We advise the code to re-instantiate the amount0 and amount1 arrays in the else branch of the FeeManager::withdrawMultipleFees function, ensuring that the assignments that ensue are correctly performed.

Alleviation (6513a21a002d422e298719b22f73a4559dfd4663):

The code will correctly re-instantiate the amt0 and amt1 arrays to permit the vault index based assignments to be performed safely, alleviating this exhibit.

Prev

DualStaking.sol (DSG-M)

Next

MultiPositionLiquidityManager.sol (MPL-M)

On this page