Uploaded files for judging

sherlock-audit · Jan 5, 2024 · 8a6e512 · 8a6e512
1 parent ef26179
commit 8a6e512
Show file tree

Hide file tree

Showing 217 changed files with 12,916 additions and 10 deletions.
diff --git a/.gitignore b/.gitignore
diff --git a/001/002.md b/001/002.md
@@ -0,0 +1,59 @@
+Unique Punch Corgi
+
+high
+
+# Miscalculation of the currentBalance and Rounding errors
+
+## Summary
+There is a potential miscalculation of the `currentBalance` and also a rounding error issue in `deposit` and `requestWithdraw` functions  at [here](https://github.com/sherlock-audit/2023-12-symm-io/blob/main/solver-vaults/contracts/SolverVaults.sol#L158)
+
+## Vulnerability Detail
+
+When users deposited a token into the vault, hes subjected to either of this calculations : 
+
+```solidity
+uint256 amountInSolverVaultTokenDecimals = solverVaultTokenDecimals >= collateralTokenDecimals
+            ? amount * (10 ** (solverVaultTokenDecimals - collateralTokenDecimals))
+            : amount / (10 ** (collateralTokenDecimals - solverVaultTokenDecimals));
+```
+depending on the `collateralDecimal` and `solverVaultTokenDecimals`
+
+and also when he request for withdraw :
+
+```solidity
+ uint256 amountInCollateralDecimals = collateralTokenDecimals >= solverVaultTokenDecimals
+            ? amount * (10 ** (collateralTokenDecimals - solverVaultTokenDecimals))
+            : amount / (10 ** (solverVaultTokenDecimals - collateralTokenDecimals));
+```
+
+The issues here is there is a potential rounding errors, lets say we have `solverVaultTokenDecimals ` of `18`  `collateralTokenDecimals ` of `6` (of course , since thats whats allowed) and   users want to deposit `100` collateral which is of ` 6 decimals`( 100e6)
+
+he will be subjected to this calculations : ` amount * (10 ** (solverVaultTokenDecimals - collateralTokenDecimals))` since  `solverVaultTokenDecimals ` is > collateralTokenDecimals (`18` and `6`) so thats  : `(10 ** (18 - 6) =  10,00,00,00,00,000` , and 100e6 * 10,00,00,00,00,000 = `100000000000000000000` equivalent to `1000`   `SolverVaultToken` for user.
+
+Now `requestWithdraw`:
+
+same calculations except this time user will be subjected to this calculation `amount / (10 ** (solverVaultTokenDecimals - collateralTokenDecimals));` because `collateralTokenDecimals` (`6`) is < `solverVaultTokenDecimals ` (`12`),
+ lets say user wants to withdraw 100(100e6)
+
+that means `100e18  /  1000000000000 ` = `0.0001`
+
+
+## Impact
+
+1. will request a withdrawal of `0.0001`  which will round to zero and is not what he's intended to 
+2. `currentDeposit` will filled so easily because there is no way to reduce the value even if user's withdrawal has been proceed
+
+## Code Snippet
+
+## Tool used
+
+Manual Review
+
+## Recommendation
+
+` For Rounding Errors`
+1. First is to put a require statement that whenever it round to zero, just revert, this is the best mitigation for me, or change the computation
+
+`For currentBalance`
+
+1. Use the `amount` that users is trying to withdraw and was the one used during deposit to ensure flexible calculations of user `in` and user `out` instead of `amountInCollateralDecimals` for the requestWithdraw`
diff --git a/001/013.md b/001/013.md
@@ -0,0 +1,111 @@
+Decent Hazel Spider
+
+medium
+
+# SolverVaults.requestWithdraw() does not check if the collateral token is paused
+
+## Summary
+The contract is meant to be used with USDT and USDC tokens on BNB, Arbitrum, Polygon, Base, opBNB, zkEVM, Optimism. Some tokens, for example USDC on Aribtrum, can be paused, meaning that their functionality is frozen. At the same time, USDT supports blacklisting accounts. `SolverVaults.requestWithdraw()` is neither checking if the collateral token is paused, nor if the user that makes the request is blacklisted, which burns the vault tokens from the user but does not allow them to get back their collateral tokens.
+
+## Vulnerability Detail
+Let's have user A call [`requestWithdraw`](https://github.com/SYMM-IO/solver-vaults/blob/4bdebbecb66e29ac18e5a5c9eda42e4cb44cdd65/contracts/SolverVaults.sol#L201-L234) when at least one of two conditions are true:
+ - user A is in the collateral token's blacklist
+ - the collateral token's functionality is currently paused
+
+Since `requestWithdraw` does not check for these conditions, the user's vault tokens will be successfully burned. 
+
+```solidity
+        SolverVaultToken(solverVaultTokenAddress).burnFrom(msg.sender, amount);
+```
+
+The request will be pushed in the queue, but it won't be processed successfully, since transferring and approving will not be available in [`acceptWithdrawRequest`](https://github.com/SYMM-IO/solver-vaults/blob/4bdebbecb66e29ac18e5a5c9eda42e4cb44cdd65/contracts/SolverVaults.sol#L236-L280)
+
+```solidity
+        IERC20(collateralTokenAddress).safeTransferFrom(
+            msg.sender,
+            address(this),
+            providedAmount
+        );
+```
+
+## Impact
+Users lose their vault tokens, but do not receive any collateral tokens in return. This is a problem, because the vault tokens are used for staking. This means that these users will lose their staking power for as long as the aforementioned conditions are true. In case that users are blacklisted, the fault is theirs, but it is possible for a user to not know that the token is paused and make a request, thus Medium severity is appropriate.
+
+## Code Snippet
+```solidity
+    function requestWithdraw(
+        uint256 amount,
+        address receiver
+    ) external whenNotPaused {
+        require(
+            SolverVaultToken(solverVaultTokenAddress).balanceOf(msg.sender) >=
+                amount,
+            "SolverVault: Insufficient token balance"
+        );
+        SolverVaultToken(solverVaultTokenAddress).burnFrom(msg.sender, amount);
+
+        uint256 amountInCollateralDecimals = collateralTokenDecimals >=
+            solverVaultTokenDecimals
+            ? amount *
+                (10 ** (collateralTokenDecimals - solverVaultTokenDecimals))
+            : amount /
+                (10 ** (solverVaultTokenDecimals - collateralTokenDecimals));
+
+        currentDeposit -= amountInCollateralDecimals;
+
+        withdrawRequests.push(
+            WithdrawRequest({
+                receiver: receiver,
+                amount: amountInCollateralDecimals,
+                status: RequestStatus.Pending,
+                acceptedRatio: 0
+            })
+        );
+        emit WithdrawRequestEvent(
+            withdrawRequests.length - 1,
+            receiver,
+            amountInCollateralDecimals
+        );
+    }
+```
+
+```solidity
+    function acceptWithdrawRequest(
+        uint256 providedAmount,
+        uint256[] memory _acceptedRequestIds,
+        uint256 _paybackRatio
+    ) external onlyRole(BALANCER_ROLE) whenNotPaused {
+        IERC20(collateralTokenAddress).safeTransferFrom(
+            msg.sender,
+            address(this),
+            providedAmount
+        );
+        require(
+            _paybackRatio >= minimumPaybackRatio,
+            "SolverVault: Payback ratio is too low"
+        );
+        uint256 totalRequiredBalance = lockedBalance;
+
+        for (uint256 i = 0; i < _acceptedRequestIds.length; i++) {
+            uint256 id = _acceptedRequestIds[i];
+            require(
+                id < withdrawRequests.length,
+                "SolverVault: Invalid request ID"
+            );
+            require(
+                withdrawRequests[id].status == RequestStatus.Pending,
+                "SolverVault: Invalid accepted request"
+            );
+            totalRequiredBalance +=
+                (withdrawRequests[id].amount * _paybackRatio) /
+                1e18;
+            withdrawRequests[id].status = RequestStatus.Ready;
+            withdrawRequests[id].acceptedRatio = _paybackRatio;
+        }
+```
+## Tool used
+
+Manual Review
+
+## Recommendation
+Change `SolverVaults.requestWithdraw()` to check if the user is blacklisted and if the collateral token is paused. If any of these is true, revert the transaction, not allowing the user to lose their tokens.
diff --git a/001/014.md b/001/014.md
@@ -0,0 +1,175 @@
+Ancient Mint Peacock
+
+high
+
+# Insolvency Risk: `SolverVaults.sol` can take on additional deposits even though they have outstanding obligations.
+
+## Summary
+
+When making a call to [`requestWithdraw(uint256,address)`](https://github.com/sherlock-audit/2023-12-symm-io/blob/60ee22a4c598220821385cfb5eee43f40aafd5f1/solver-vaults/contracts/SolverVaults.sol#L201), [`currentDeposit`](https://github.com/sherlock-audit/2023-12-symm-io/blob/60ee22a4c598220821385cfb5eee43f40aafd5f1/solver-vaults/contracts/SolverVaults.sol#L75) is immediately reduced by the total value of the [`amountInCollateralDecimals`](https://github.com/sherlock-audit/2023-12-symm-io/blob/60ee22a4c598220821385cfb5eee43f40aafd5f1/solver-vaults/contracts/SolverVaults.sol#L212). This enables the vault to continue accepting additional deposits even though the funds being requested to withdraw are _still_ deposited, resulting in an inadvertent breach of the [`depositLimit`](https://github.com/sherlock-audit/2023-12-symm-io/blob/60ee22a4c598220821385cfb5eee43f40aafd5f1/solver-vaults/contracts/SolverVaults.sol#L74).
+
+## Vulnerability Detail
+
+In the demonstration below, we show that a [`SolverVault`](https://github.com/sherlock-audit/2023-12-symm-io/blob/60ee22a4c598220821385cfb5eee43f40aafd5f1/solver-vaults/contracts/SolverVaults.sol) can take on double the maximum intended [`depositLimit`](https://github.com/sherlock-audit/2023-12-symm-io/blob/60ee22a4c598220821385cfb5eee43f40aafd5f1/solver-vaults/contracts/SolverVaults.sol#L74), due to the vault incorrectly reporting a safe deposit balance.
+
+Further, this excess deposit balance can be misappropriated to meet existing financial obligations.
+
+```solidity
+// SPDX-License-Identifier: UNLICENSED
+pragma solidity ^0.8.13;
+
+import {Test} from "forge-std/Test.sol";
+import {console} from "forge-std/console.sol";
+
+import {SolverVault} from "../contracts/SolverVaults.sol";
+import {SolverVaultToken} from "../contracts/SolverVaultToken.sol";
+import {MockERC20} from "../contracts/test/MockERC20.sol";
+import {MockSymmio} from "../contracts/test/MockSymmio.sol";
+
+contract SolverTest is Test {
+
+    /* actors */
+    address private constant _DEPLOYER = address(0x69);
+    address private constant _SOLVER = address(0x420);
+    address private constant _ALICE = address(0x1559);
+    address private constant _BOB = address(0x721);
+
+    function test_Insolvency() public {
+
+        // Initialize a simple vault and distribute some test tokens.
+        // Importantly, `maxDeposit` is `1e18` and `minPaybackRatio`
+        // is also `1e18`.
+        vm.startPrank(_DEPLOYER);
+
+            MockERC20 mockERC20 = new MockERC20(18);
+            MockSymmio mockSymmio = new MockSymmio(address(mockERC20));
+
+            SolverVaultToken solverVaultToken = new SolverVaultToken();
+            SolverVault solverVault = new SolverVault();
+
+            solverVault.initialize(
+                address(mockSymmio),
+                address(solverVaultToken),
+                _SOLVER,
+                1e18,
+                1e18
+            );
+
+            mockERC20.approve(address(solverVault), type(uint256).max);
+
+            mockERC20.mint(_ALICE, 1e18);
+            mockERC20.mint(_BOB, 1e18);
+
+            solverVaultToken.grantRole(mockERC20.MINTER_ROLE(), address(solverVault));
+
+        vm.stopPrank();
+
+        vm.startPrank(_ALICE);
+
+            mockERC20.approve(address(solverVault), type(uint256).max);
+            solverVault.deposit(1e18);
+
+        vm.stopPrank();
+
+        // Here, Bob attempts to make a deposit, but due to Alice's interaction,
+        // the `maxDeposit` has been reached causing the operation to correctly
+        // fail.
+        vm.startPrank(_BOB);
+
+            mockERC20.approve(address(solverVault), type(uint256).max);
+
+            vm.expectRevert("SolverVault: Deposit limit reached");
+            solverVault.deposit(1e18);
+
+        vm.stopPrank();
+
+        // Let's have the deployer do something with Alice's funds.
+        vm.prank(_DEPLOYER);
+          solverVault.depositToSymmio(1e18);
+
+        // Next, Alice requests a withdrawal:
+        vm.startPrank(_ALICE);
+            solverVaultToken.approve(address(solverVault), type(uint256).max);
+            solverVault.requestWithdraw(solverVaultToken.balanceOf(_ALICE), _ALICE);
+        vm.stopPrank();
+
+        // At this stage, the vault has an obligation to pay Alice their
+        // balance, as their funds are still deposited.
+        // However, the vault continues to take on additional deposits in
+        // excess of the `maxDeposits`:
+        vm.prank(_BOB);
+            solverVault.deposit(1e18) /* should_revert */;
+
+        // Next, let's have the deployer redeem Alice's request.
+        // Notice here, the deployer doesn't have to provide ANY
+        // excess value to the vault, they can merely redeem Alice's
+        // request using Bob's excess funds.
+        vm.prank(_DEPLOYER);
+            solverVault.acceptWithdrawRequest(0, new uint256[](1), 1e18);
+
+    }
+    
+}
+```
+
+Inferring from this sequence, `_BOB` may then [`requestWithdraw(uint256,address)`](https://github.com/sherlock-audit/2023-12-symm-io/blob/60ee22a4c598220821385cfb5eee43f40aafd5f1/solver-vaults/contracts/SolverVaults.sol#L201), allowing the vault to take on further additional deposits and continue paying with additional user stake.
+
+## Impact
+
+This means that solvers which are currently unable to meet their financial obligations may take on excess deposits, and use these to pay back open requests to withdraw, resulting in:
+
+1. The unintentional rehypothecation of user funds when paying back existing obligations.
+2. The vault will incorrectly report as healthy even under extreme conditions, such as a bank run.
+3. Vaults can accept more deposits than intended.
+4. Deposits by users may not end up be meaningfully invested, leading to a loss of yield.
+
+## Code Snippet
+
+```solidity
+function requestWithdraw(
+    uint256 amount,
+    address receiver
+) external whenNotPaused {
+    require(
+        SolverVaultToken(solverVaultTokenAddress).balanceOf(msg.sender) >=
+            amount,
+        "SolverVault: Insufficient token balance"
+    );
+    SolverVaultToken(solverVaultTokenAddress).burnFrom(msg.sender, amount);
+
+    uint256 amountInCollateralDecimals = collateralTokenDecimals >=
+        solverVaultTokenDecimals
+        ? amount *
+            (10 ** (collateralTokenDecimals - solverVaultTokenDecimals))
+        : amount /
+            (10 ** (solverVaultTokenDecimals - collateralTokenDecimals));
+
+    currentDeposit -= amountInCollateralDecimals;
+
+    withdrawRequests.push(
+        WithdrawRequest({
+            receiver: receiver,
+            amount: amountInCollateralDecimals,
+            status: RequestStatus.Pending,
+            acceptedRatio: 0
+        })
+    );
+    emit WithdrawRequestEvent(
+        withdrawRequests.length - 1,
+        receiver,
+        amountInCollateralDecimals
+    );
+}
+```
+
+## Tool used
+
+Foundry
+
+## Recommendation
+
+To model a solution on the existing implementation, the [`currentDeposit`](https://github.com/sherlock-audit/2023-12-symm-io/blob/60ee22a4c598220821385cfb5eee43f40aafd5f1/solver-vaults/contracts/SolverVaults.sol#L75) should only be decreased for a user's contribution during a successful call to [`acceptWithdrawRequest(uint256,uint256[],uint256)`](https://github.com/sherlock-audit/2023-12-symm-io/blob/60ee22a4c598220821385cfb5eee43f40aafd5f1/solver-vaults/contracts/SolverVaults.sol#L236).
+
+Alternatively, optimistic modifications to [`currentDeposit`](https://github.com/sherlock-audit/2023-12-symm-io/blob/60ee22a4c598220821385cfb5eee43f40aafd5f1/solver-vaults/contracts/SolverVaults.sol#L75) upon a withdrawal request may indeed still be made safely if the intention is to increase the liveliness of deposits.
+
+If the difference between the initial deposit and the user's accepted [`minimumPaybackRatio`](https://github.com/sherlock-audit/2023-12-symm-io/blob/60ee22a4c598220821385cfb5eee43f40aafd5f1/solver-vaults/contracts/SolverVaults.sol#L73) is less than `1e18`, that is to say the user is willing to accept less than they originally deposited, this delta could be immediately subtracted from [`currentDeposit`](https://github.com/sherlock-audit/2023-12-symm-io/blob/60ee22a4c598220821385cfb5eee43f40aafd5f1/solver-vaults/contracts/SolverVaults.sol#L75) since the user has accepted the terms of withdrawal. This would require the remaining deposit balance to be subtracted in [`acceptWithdrawRequest(uint256,uint256[],uint256)`](https://github.com/sherlock-audit/2023-12-symm-io/blob/60ee22a4c598220821385cfb5eee43f40aafd5f1/solver-vaults/contracts/SolverVaults.sol#L236).