yAudit Rysk vault Review

Review Resources:

• Codebase
• docs

Auditors:

• spalen
• panda

Table of Contents

1. Review Summary
2. Scope
3. Code Evaluation Matrix
4. Findings Explanation
5. Low Findings
   1. 1. Low - takeGas() should return excess ETH sent by mistake
   2. 2. Low - Unintended queue item skipping in processQueue()
   3. 3. Low - Verify deposit amount is above minimum amount
6. Gas Saving Findings
   1. 1. Gas - Structure can be packed into fewer storage slots
   2. 2. Gas - Use a circular buffer instead of an indefinitely growing queue
   3. 3. Gas - Optimize queueCounter
7. Informational Findings
   1. 1. Informational - Unused import
   2. 2. Informational - Typos
   3. 3. Informational - Refactor duplicated require()/revert() checks into a modifier or function
   4. 4. Informational - Unused error string
   5. 5. Informational - Setters should have boundaries
   6. 6. Informational - approvedSigner can’t be updated
   7. 7. Informational - Replace magic numbers with constants
   8. 8. Informational - Use OpenZeppelin forceApprove() instead of approve
   9. 9. Informational - Missing event emits
   10. 10. Informational - Change the value of processQueueGasUsage
8. Final remarks

Review Summary

Rysk vault

Rysk provides a vault to wrap deposits into the Ciao contract.

The contracts of the Rysk Repo were reviewed over 3 days. The code review was performed by 2 auditors between April 16th and April 19th, 2024. The repository was under active development during the review, but the review was limited to the latest commit at the start of the review. This was commit b7538f59f97846eda129435b7c0debe473360bf3 for the Risk vault repo.

Scope

The scope of the review consisted of the following contracts at the specific commit:

src
├── Vault.sol
├── interfaces
│   ├── Errors.sol
│   ├── Events.sol
│   ├── IAddressManifest.sol
│   ├── IBlast.sol
│   ├── IBlastPoints.sol
│   ├── ICiao.sol
│   ├── IProductCatalogue.sol
│   ├── IVault.sol
│   └── WETHRebasing.sol

After the findings were presented to the Rysk team, fixes were made and included in several PRs.

This review is a code review to identify potential vulnerabilities in the code. The reviewers did not investigate security practices or operational security and assumed that privileged accounts could be trusted. The reviewers did not evaluate the security of the code relative to a standard or specification. The review may not have identified all potential attack vectors or areas of vulnerability.

yAudit and the auditors make no warranties regarding the security of the code and do not warrant that the code is free from defects. yAudit and the auditors do not represent nor imply to third parties that the code has been audited nor that the code is free from defects. By deploying or using the code, Rysk vault and users of the contracts agree to use the code at their own risk.

Code Evaluation Matrix

Findings Explanation

Findings are broken down into sections by their respective impact:

• Critical, High, Medium, Low impact
  • These are findings that range from attacks that may cause loss of funds, impact control/ownership of the contracts, or cause any unintended consequences/actions that are outside the scope of the requirements.
• Gas savings
  • Findings that can improve the gas efficiency of the contracts.
• Informational
  • Findings including recommendations and best practices.

Low Findings

1. Low - takeGas() should return excess ETH sent by mistake

In case a user sends significantly more ETH than necessary to cover future gas usage, the contract should return the excess ETH.

Technical Details

299 |        // ETH cost of the transaction using the stored gas amount.
300 |        uint256 fee = processQueueGasUsage * gasPrice;
301 |
302 |        // Check that the msg.value is equal or more than the fee.
303 |        if (msg.value < fee) revert(Errors.feeTooLow);
304 |
305 |        // Transfer fee to the external account EOA.
306 |        (bool sent,) = payable(approvedSigner).call{value: msg.value}("");
307 |        if (!sent) revert(Errors.feeTransferFailed);

Vault.sol#L300-L307

Impact

Low. There is a risk that users might inadvertently overpay.

Recommendation

Define a threshold, for example, 5% above the fee. If this threshold is exceeded, refund all ETH above the fee.

Developer Response

implemented. changes

2. Low - Unintended queue item skipping in processQueue()

processQueue() function, there is a vulnerability that allows a approvedSigner to inadvertently skip future queue positions or alter a previously processed queueItem. This occurs when the amountToReceive parameter is passed as zero, permitting any value of queuePosition to be used without validation. Although skipping is intentional when amountToReceive is zero, the nextToProcess variable is always incremented, which can lead to unintentional skipping of important future queue items that won’t be processable.

Technical Details

File: Vault.sol
185 |    function processQueue(uint256 queuePosition, uint256 amountToReceive) external {
186 |        if (msg.sender != approvedSigner) revert(Errors.notApprovedSigner);
187 |        // Get the action data from the queue.
188 |        QueueItem storage queueItem = queue[nextToProcess];
189 |        if (queueItem.sender == address(0)) revert(Errors.emptyQueueItem);
190 |
191 |        if (amountToReceive != 0) {
192 |            // check queuePosition matches the next position to be processed
193 |             if (queuePosition != nextToProcess) {
194 |                 revert(Errors.invalidQueuePosition);
195 |            }
196 |
197 |            // process this position in the queue. Skips if fails or reverts
198 |            try this.processSingleQueuePosition(queueItem, amountToReceive) {
199 |                queueItem.state = ItemState.Executed;
200 |            } catch {
201 |                queueItem.state = ItemState.Skipped;
202 |            }
203 |        } else {
204 |            // intentionally skip this position
205 |             queueItem.state = ItemState.Skipped;
206 |         }
207 |
208 |         // increment next position to process
209 |         nextToProcess++;
210 |     }

Vault.sol#L185-L201

Impact

Low. A wrong skipped item will prevent queued items from ever being executed.

Recommendation

Remove the queuePosition parameter and always process the nextToProcess item.

Developer Response

Moved queuePosition check on L193 to outside the if block immediately above it. changes

3. Low - Verify deposit amount is above minimum amount

The user can use any amount in deposit(), but Ciao has defined minimum deposit amount per each token. This can lead to failed transactions in the queue and users paying gas.

Technical Details

Ciao has the function minDepositAmount() to get the minimum deposit amount. Reject the users who are trying to deposit below that amount to save some gas costs for the users.

Impact

Low. The user will pay the deposit gas fee even though the deposit call to Ciao will fail.

Recommendation

In deposit() verify that the amount is above minimum value defined in Ciao for desired token.

if (ciao.minDepositAmount(token) > amount) revert(Errors.insufficientDepositAmount);

Developer Response

implemented changes

Gas Saving Findings

1. Gas - Structure can be packed into fewer storage slots

The QueueItem can be organized to save 20000 gas on every structure added to the queue.

Technical Details

15 | struct QueueItem {
16 |     Action action;
17 |     address sender;
18 |     address token;
19 |     uint256 amount; // for Deposits, this is core collateral. For Withdrawals it is LP shares.
20 |     ItemState state;
21 | }

Can be restructured following this order, to have sender, action and state under the same storage space.

 * uint256 amount; // (256 bits)
 * address sender; // (160 bits)
 * enum IVault.Action action; // (8 bits)
 * enum IVault.ItemState state; // (8 bits)
 * address token; // (160 bits)

IVault.sol#L15

Impact

Gas savings.

Recommendation

Change the structure definition.

Developer Response

implemented. changes

2. Gas - Use a circular buffer instead of an indefinitely growing queue

Managing a FIFO queue efficiently in blockchain applications is essential due to the high cost of gas associated with state changes. A solution to reduce gas usage and prevent the queue from growing indefinitely is to replace the queue with a circular buffer.

Technical Details

The circular buffer method allows efficient queue management by using a fixed-size array and two pointers to denote the front and rear of the queue. This approach avoids costly array-shifting operations.

Here is an idea of a queue structure and function associated.

uint256 constant MAX_SIZE=500;
QueueItem[MAX_SIZE] queue;
uint256 front;
uint256 rear;
uint256 size;

function enqueue(Queue storage queue, QueueItem item) public {
    require(size < MAX_SIZE, "Queue is full");
    rear = (rear + 1) % MAX_SIZE;
    queue[rear] = item;
    size++;
}

function dequeue(Queue storage queue) public returns (QueueItem) {
    require(size > 0, "Queue is empty");
    uint item = data[queue.front];
    front = (front + 1) % capacity;
    size--;
    return item;
}

The gas savings become effective after the queue is filled once. Therefore, determining the optimal size of the queue to maximize gas efficiency while ensuring sufficient capacity is crucial.

Impact

Gas savings.

Recommendation

Consider a circular buffer, if not storing the processed data is possible.

Developer Response

acknowledged.

3. Gas - Optimize queueCounter

queueCounter can be optimized in deposit and withdraw functions.

Technical Details

queueCounter is first increased by one and later decreased by one. Using additional variable to store counter can save gas.

Impact

Gas savings.

Recommendation

Change deposit() to following:

uint256 currentItem = queueCounter++;
queue[currentItem] =
    QueueItem(Action.Deposit, msg.sender, token, amount, ItemState.Pending);

emit Events.AddedToQueue(queue[currentItem], queueCounter, nextToProcess);

Change withdraw() to following:

uint256 currentItem = queueCounter++;
queue[currentItem] =
    QueueItem(Action.Withdraw, msg.sender, token, amount, ItemState.Pending);

emit Events.AddedToQueue(queue[currentItem], queueCounter, nextToProcess);

Gas savings data from provided tests:

test_Fail_userMakesDepositNotEnoughGas() (gas: -10 (-0.008%))
test_Happy_userMakesWithdrawalRequest(uint8,uint8,uint128,uint128,uint128) (gas: -192 (-0.033%))
test_Happy_userMakesDepositAndProcessed(uint8,uint128,uint128) (gas: -192 (-0.034%))
test_Fail_userMakesWithdrawalInsufficientShares() (gas: -192 (-0.035%))
test_Happy_ProcessDeposit() (gas: -192 (-0.037%))
test_Happy_ProcessMultipleDeposits() (gas: -384 (-0.046%))
test_Happy_processWithdrawalBlacklistedUser() (gas: -384 (-0.046%))
test_Happy_ProcessUserWithdrawal() (gas: -384 (-0.048%))
test_Happy_ProcessDepositCiaoDepositError() (gas: -192 (-0.049%))
test_Happy_ProcessDepositAndWithdrawal() (gas: -576 (-0.060%))
test_Happy_userWithdrawal() (gas: -384 (-0.062%))
test_Happy_ProccessDepositHardCapReached() (gas: -192 (-0.064%))
test_Happy_ProcessDepositButUserRemovesAllowance() (gas: -192 (-0.070%))
test_Happy_ProcessMultipleUserWithdrawals() (gas: -960 (-0.071%))
test_Happy_SkipQueuePosition() (gas: -192 (-0.085%))
test_Happy_userMultipleWithdrawals() (gas: -960 (-0.086%))
test_Fail_ProcessDepositInvalidQueuePosition() (gas: -192 (-0.105%))
test_Happy_userMakesDepositRequest() (gas: -192 (-0.106%))
test_Happy_UserMakeDeposit(uint8,uint128) (gas: -192 (-0.110%))
test_Happy_userMakesDepositRequestTwoAssets() (gas: -384 (-0.125%))
test_Happy_userMakesWithdrawalAndProcessed(uint8,uint8,uint128,uint128,uint128,uint128) (gas: -1384 (-0.163%))
Overall gas change: -7922 (-0.064%)

Developer Response

implemented. changes

Informational Findings

1. Informational - Unused import

The identifier is imported but never used within the file.

Technical Details

File: src/Vault.sol

6 | import {IAddressManifest} from "src/interfaces/IAddressManifest.sol";

Vault.sol#L6

Impact

Informational.

Recommendation

Remove the unused import.

Developer Response

Fixed. changes

2. Informational - Typos

Typos were found in the codebase.

Technical Details

File: src/Vault.sol

// @audit: processs should be processes, process
56 | /// @notice the queue for the backend to processs

// @audit: responisble should be responsible
134 | /// @dev the back-end is responisble for calling ProcessQueue with correct parameters.

// @audit: responisble should be responsible
153 | /// @dev the back-end is responisble for calling ProcessQueue with correct parameters.

// @audit: oustanding should be outstanding
274 | // decrease total oustanding shares balance

Vault.sol 56, 134, 153, 274

Impact

Informational.

Recommendation

Fix typos.

Developer Response

Fixed. changes

3. Informational - Refactor duplicated require()/revert() checks into a modifier or function

It is recommended to refactor duplicated require()/revert() checks into a modifier.

Technical Details

File: src/Vault.sol

// @audit: Other occurrence found: src/Vault.sol:159
139 | if (approvedTokens[token] == false) revert(Errors.invalidToken);

// @audit: Other occurrence found: src/Vault.sol:139
159 | if (approvedTokens[token] == false) revert(Errors.invalidToken);

Vault.sol 139, 159

Impact

Informational.

Recommendation

You should use a modifier to improve code clarity.

Developer Response

Acknowledged.

4. Informational - Unused error string

amountIsZero is not used.

Technical Details

File: src/interfaces/Errors.sol

33 | string internal constant amountIsZero = "Amount cannot be zero";

Errors.sol#L33

Impact

Informational.

Recommendation

Make sure it’s not meant to be used and remove it if not.

Developer Response

Removed. changes

5. Informational - Setters should have boundaries

It’s recommended to have boundaries for the values set by the different update methods.

Technical Details

File: Vault.sol
214 |    function updateGas(uint256 _processQueueGasUsage) external onlyOwner {
215 |        processQueueGasUsage = _processQueueGasUsage;
216 |
217 |        emit Events.GasUpdated(_processQueueGasUsage);
218 |    }
219 |
220 |    function updateHardCap(uint256 _hardCap) external onlyOwner {
221 |        hardCap = _hardCap;
222 |        emit Events.HardCapUpdated(_hardCap);
222 |    }
223 |
224 |    function updateVaultFee(uint256 _vaultFee) external onlyOwner {
225 |        vaultFee = _vaultFee;
226 |        emit Events.VaultFeeUpdated(_vaultFee);
227 |    }

Vault.sol#L214-L228

Impact

Informational.

Recommendation

Add hard limits to prevent values from being set to low or too high.

Developer Response

Acknowledged.

6. Informational - approvedSigner can’t be updated

approvedSigner is not updatable, it might be necessary to update it, we recommend adding a setter to approvedSigner.

Technical Details

A setter might be missing for approvedSigner, without it the contract will have to be updated before updating approvedSigner.

Impact

Informational.

Recommendation

Add a setter to approvedSigner.

Developer Response

Created function to update approved signer. changes

7. Informational - Replace magic numbers with constants

Constant variables should be used in place of magic numbers to prevent typos and better explain such values.

Technical Details

For example, the unexplained number is 10000 can be replaced with the constant private variable maximum basis points. Another value is 0 used for subAccountId in the Ciao protocol when approving the signer and for call deposit to the Ciao.

Impact

Informational.

Recommendation

Use constant variables instead of magic numbers. This will not change gas consumption.

Developer Response

Fixed. changes

8. Informational - Use OpenZeppelin forceApprove() instead of approve

In case a token doesn’t comply with the ERC20 approve method it’s recommended to use forceApprove() from SafeERC20

Technical Details

File: Vault.sol
231 |        approvedTokens[_token] = _approved;
232 |        if (_approved) {
233 |            IERC20(_token).approve(address(ciao), type(uint256).max);
234 |        } else {
235 |            IERC20(_token).approve(address(ciao), 0);
236 |        }

Vault.sol#L231-L236

Impact

Informational.

Recommendation

Use forceApprove() instead of approve().

Developer Response

implemented. changes

9. Informational - Missing event emits

Events can assist with analyzing the on-chain history of contracts and are therefore beneficial to add in important functions.

Technical Details

Function configureBlastGovernor() is changing state variable but do not have an event to track it.

Impact

Informational.

Recommendation

Add event to the function configureBlastGovernor().

Developer Response

acknowledged. Will likely never happen and governor updates for this contract are handled externally anyway under normal circumstances.

10. Informational - Change the value of processQueueGasUsage

The initial value of variable processQueueGasUsage is set to 2_000_000. This number is too high if we compare it with the gas report from the tests.

Technical Details

Gas report values for the worst case scenario is 318_246.

| Function Name                | min             | avg    | median | max    | # calls |
| processQueue                 | 2951            | 203161 | 315009 | 318246 | 1046    |

Impact

Informational.

Recommendation

Set lower value for processQueueGasUsage.

Developer Response

acknowledged - will update when we have more accurate data taking L1 calldata costs into account

Final remarks

The MM Vault enables users to deposit and get shares of strategies on 100x Ciao protocol managed by Rysk. The strategies are managed by an off-chain system. Deposits and withdrawals are also handled by the off-chain system which puts a lot of trust in off-chain management.