Uploaded files for judging

sherlock-audit · May 24, 2024 · 15c4cf4 · 15c4cf4
1 parent 3afc6cc
commit 15c4cf4
Show file tree

Hide file tree

Showing 231 changed files with 17,140 additions and 10 deletions.
diff --git a/.gitignore b/.gitignore
diff --git a/001.md b/001.md
@@ -0,0 +1,36 @@
+Scruffy Sandstone Parrot
+
+high
+
+# Controlled Delegatecall
+
+## Summary
+There is a controlled delegatecall in`SophonFarmingProxy::receive` which is marked as payable but no proper safeguards have been used.
+
+## Vulnerability Detail
+Inside `SophonFarmingProxy::receive` the `delegatecall` is used to forward the received Ether to the `implementation` contract but but lacks a data argument. This setup is inherently risky because it can lead to a potential reentrancy vulnerability since the `delegatecall` relies entirely on the `implementation` contract to decide what action to take.
+
+## Impact
+If the `implementation` contract is malicious or compromised, it could potentially call back into the `SophonFarmingProxy` after receiving Ether, exploiting the reentrancy vulnerability. This could allow the attacker to drain funds from the proxy contract or manipulate its state in unintended ways.
+
+## Code Snippet
+https://github.com/sherlock-audit/2024-05-sophon/blob/main/farming-contracts/contracts/proxies/SophonFarmingProxy.sol#L10-L13
+
+## Tool used
+Manual Review + Slither
+
+## Recommendation
+Despite of the controlled nature of the upgrade mechanism in `Upgradeable2Step`, mitigating the reentrancy vulnerability in the `receive` function requires careful consideration. One approach is to implement Reentrancy Guard from Openzeppelin in the `SophonFarmingProxy` contract to prevent recursive calls. This can be done by adding a modifier that checks if the contract is currently being called recursively:
+
+```javascript
+import {ReentrancyGuard} from "@openzeppelin/contracts/utils/ReentrancyGuard.sol"
+
+contract SophonFarmingProxy is Proxy2Step, ReentrancyGuard {
+    constructor(address impl_) Proxy2Step(impl_) {}
+
+    receive() external override payable nonReentrant {
+        (bool success,) = implementation.delegatecall("");
+        require(success, "subcall failed");
+    }
+   ```
+  This will inherit ReentrancyGuard in the contract and 
diff --git a/001/011.md b/001/011.md
@@ -0,0 +1,40 @@
+Bent Watermelon Eagle
+
+medium
+
+# Adding or changing a pool without `massUpdatePools` will result in either too many or too little rewards than intended
+
+## Summary
+
+Functions `set` and `add` allow the owner to change existing pool's configuration, or add a new one. They both have `bool _withUpdate` parameter, which if `true` calls `massUpdatePools` before changing the config. The issue is that every time `set` or `add` are called with  `_withUpdate = false`, all pools will have incorrect rewards between their last update and current timestamp, as the change to `allocPoints` and `totalAllocPoints` would apply retrospectively.
+
+## Vulnerability Detail
+
+## Proof of Concept
+
+There's 2 empty pools: A and B, `allocPointA = allocPointB = 50`; `_pointsPerBlock = 30e18`;
+
+1. Alice stakes worth of 100 USD in pool A
+2. Bob stakes worth of 100 USD in pool B
+3. 1000 blocks later owner adds pool C with `allocPoint = 50`,  `_withUpdate = false`
+4. Alice frontruns owner's txn with `updatePool(A)`: `pointRewardA = _pointsPerBlock * blocksPassed * allocPointA / totalAlloc = 30e18 * 1000 * 50 / 100 = 15_000e18`
+5. Owner's txn is mined
+
+Now, updatePool(B) will result in: `pointRewardB = _pointsPerBlock * blocksPassed * allocPointB / totalAlloc = 30e18 * 1000 * 50 / 150 = 10_000e18`. So all stakers of pool B will receive 1/3 less rewards than all stakers of A, despite their pools having the same weight.
+
+On the other hand, If owner used `_withUpdate = true`, both Alice and Bob would have received 1/2 of rewards for the first 1000 blocks, as they should.
+
+Similarly, if a pool is `set` to a higher `allocPoint` without `massUpdatePools`, all stakers of that pool will receive more rewards than they should, and the contract in aggregate may temporarily emit more rewards than `_pointsPerBlock` (if other pools were updated later than the changed one).
+
+## Impact
+
+Too little / too many rewards for stakers whose pools were not updated right before functions `set` or `add` were called.
+
+## Code Snippet
+https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L427-L428
+## Tool used
+
+Manual Review
+
+## Recommendation
+Make `massUpdatePools` mandatory for `SophonFarming#add` and `SophonFarming#set`, instead of optional.
diff --git a/001/012.md b/001/012.md
@@ -0,0 +1,151 @@
+Proper Admiral Dalmatian
+
+medium
+
+# Conditional use of `_withUpdate` in `SophonFarming::set()` and `SophonFarming::add()` could lead to improper reward calculation
+
+## Summary
+
+In the `SophonFarming` contract, the `totalAllocPoint` variable is used to correctly determine what will be the portion that each pool would get from the total reward, making it detrimental to the reward calculation. This means that, whenever the `totalAllocPoint` is updated without updating the pending reward first, the reward calculation will be incorrect.
+
+## Vulnerability Detail
+
+https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L153
+https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L195
+
+In the `add()` and `set()` functions shown above, if `_withUpdate` is set to false, the `totalAllocPoint` variable will be modified without updating the rewards (`massUpdatePools()`).
+
+## Impact
+
+This vulnerability could lead to improper reward calculation, resulting in a loss of funds for the users.
+
+## Code Snippet
+
+```solidity
+    function add(uint256 _allocPoint, address _lpToken, string memory _description, bool _withUpdate) public onlyOwner returns (uint256) {
+        if (poolExists[_lpToken]) {
+            revert PoolExists();
+        }
+        if (isFarmingEnded()) {
+            revert FarmingIsEnded();
+        }
+@>        if (_withUpdate) { // conditional logic
+            massUpdatePools();
+        }
+        uint256 lastRewardBlock =
+            getBlockNumber() > startBlock ? getBlockNumber() : startBlock;
+@>        totalAllocPoint = totalAllocPoint + _allocPoint; // update totalAllocPoint without updating rewards if the conditional is false
+        poolExists[_lpToken] = true;
+...
+    }
+```
+
+```solidity
+    function set(uint256 _pid, uint256 _allocPoint, bool _withUpdate) public onlyOwner {
+        if (isFarmingEnded()) {
+            revert FarmingIsEnded();
+        }
+@>        if (_withUpdate) { // conditional logic
+            massUpdatePools();
+        }
+
+        PoolInfo storage pool = poolInfo[_pid];
+        address lpToken = address(pool.lpToken);
+        if (lpToken == address(0) || !poolExists[lpToken]) {
+            revert PoolDoesNotExist();
+        }
+@>        totalAllocPoint = totalAllocPoint - pool.allocPoint + _allocPoint; // update totalAllocPoint without updating rewards if the conditional is false
+        pool.allocPoint = _allocPoint;
+...
+    }
+```
+
+## Tool used
+
+Manual Review
+
+## Proof of concept
+
+1. Let's assume that on block `1000000`, we have `pointPerBlock = 5`, `totalAllocPoint = 5000`, and `pool.allocPoint = 500` and all rewards are updated.
+2. The owner uses `add()` on block `1100000`, increasing the `totalAllocPoint` to `10000` without updating the pool rewards (`_withUpdate = false`).
+3. All pools are again updated on block `1200000`.
+
+From the above scenario, the `pointReward` from block `1000000` to `1200000` will be:
+
+```solidity
+pointRewardAt1200000 = blockMultiplier * pointsPerBlock * pool.allocPoint / totalAllocPoint;
+
+pointRewardAt1200000 = 200000 * 5 * 500 / 10000 = 50000
+```
+
+However, the rewards should be calculated by accounting for the original `totalAllocPoint` value during the period when it is not yet updated as follows:
+
+```solidity
+pointRewardAt1100000 = blockMultiplier * pointsPerBlock * pool.allocPoint / totalAllocPoint
+
+pointRewardAt1100000 = 100000 * 5 * 500 / 5000 = 50000 
+```
+
+Then at `110000`, the rewards are properly updates, so at `1200000`:
+
+```solidity
+pointRewardFrom110000to120000 = blockMultiplier * pointsPerBlock * pool.allocPoint / totalAllocPoint
+
+pointRewardFrom110000to120000 = 100000 * 5 * 500 / 10000 = 25000
+```
+
+We can now see that the reward should be `50000 + 25000 = 75000`, differing from the one calculated without updating the rewards with `25000`.
+
+## Recommendation
+
+Remove the conditional logic (`bool _withUpdate`) and always update the rewards, before updating `totalAllocPoint` in the `add()` and `set()` functions.
+
+<details>
+
+<summary>Diff</summary>
+
+```diff
+@@ -147,19 +147,17 @@ contract SophonFarming is Upgradeable2Step, SophonFarmingState {
+      * @param _allocPoint alloc point for new pool
+      * @param _lpToken lpToken address
+      * @param _description description of new pool
+-     * @param _withUpdate True will update accounting for all pools
+      * @return uint256 The pid of the newly created asset
++     * // audit - missing zero address check for _lpToken
+      */
+-    function add(uint256 _allocPoint, address _lpToken, string memory _description, bool _withUpdate) public onlyOwner returns (uint256) {
++    function add(uint256 _allocPoint, address _lpToken, string memory _description) public onlyOwner returns (uint256) {
+         if (poolExists[_lpToken]) {
+             revert PoolExists();
+         }
+         if (isFarmingEnded()) {
+             revert FarmingIsEnded();
+         }
+-        if (_withUpdate) {
+-            massUpdatePools();
+-        }
++        massUpdatePools();
+         uint256 lastRewardBlock =
+             getBlockNumber() > startBlock ? getBlockNumber() : startBlock;
+         totalAllocPoint = totalAllocPoint + _allocPoint;
+@@ -190,15 +188,12 @@ contract SophonFarming is Upgradeable2Step, SophonFarmingState {
+      * @notice Updates the given pool's allocation point. Can only be called by the owner.
+      * @param _pid The pid to update
+      * @param _allocPoint The new alloc point to set for the pool
+-     * @param _withUpdate True will update accounting for all pools
+      */
+-    function set(uint256 _pid, uint256 _allocPoint, bool _withUpdate) public onlyOwner {
++    function set(uint256 _pid, uint256 _allocPoint) public onlyOwner {
+         if (isFarmingEnded()) {
+             revert FarmingIsEnded();
+         }
+-        if (_withUpdate) {
+-            massUpdatePools();
+-        }
++        massUpdatePools();
+
+         PoolInfo storage pool = poolInfo[_pid];
+         address lpToken = address(pool.lpToken);
+```
+
+</details>
diff --git a/001/014.md b/001/014.md
@@ -0,0 +1,61 @@
+Mean Shamrock Lark
+
+medium
+
+# [M-1] `SophonFarming::_pendingPoints` - Division Before Multiplication Causes Precision Loss
+
+## Summary
+The `_pendingPoints` function in the `SophonFarming` contract performs division before multiplication, which can lead to precision loss when dealing with small numbers or when the divisor is greater than the dividend. 
+
+## Vulnerability Detail
+Proof of Concept:
+blockMultiplier = 1000
+pointsPerBlock = 1e18
+pool.allocPoint = 1
+totalAllocPoint = 1000000
+lpSupply = 1e18
+
+Plugging these values into the original code:
+```javascript
+uint256 pointReward = 1000 * 1e18 * 1 / 1000000;
+accPointsPerShare = pointReward * 1e18 / 1e18 + accPointsPerShare;
+```
+
+Simplifying the calculations:
+```javascript
+uint256 pointReward = 1e15 / 1000000;
+accPointsPerShare = pointReward + accPointsPerShare;
+```
+
+## Impact
+The precision loss caused by performing division before multiplication can result in incorrect calculations of pending points for users in the `_pendingPoints` function. This can lead to users receiving fewer points than they should, affecting the fairness and accuracy of the reward distribution in the SophonFarming protocol.
+
+The value of `pointReward` will be truncated to 0 because the division by `1000000` is performed before the multiplication by `1e18`. As a result, `accPointsPerShare` will not be incremented, leading to a loss of precision.
+
+However, if the order of operations is changed to perform multiplication before division, the precision loss can be mitigated:
+```javascript
+uint256 pointReward = 1000 * 1e18 * 1 * 1e18 / 1000000 / 1e18;
+accPointsPerShare = pointReward + accPointsPerShare;
+```
+Simplifying the calculations:
+```javascript
+uint256 pointReward = 1e33 / 1000000 / 1e18;
+accPointsPerShare = pointReward + accPointsPerShare;
+```
+In this case, the value of pointReward will be correctly calculated as 1e15, and accPointsPerShare will be incremented accordingly, preserving the precision.
+
+## Code Snippet
+The vulnerability can be found here:
+https://github.com/sherlock-audit/2024-05-sophon/blob/main/farming-contracts/contracts/farm/SophonFarming.sol?plain=1#L367-L377
+
+## Tool used
+Math
+Manual Review
+
+## Recommendation
+To mitigate this vulnerability, the order of operations in the _pendingPoints function should be changed to perform multiplication before division. Modify the affected lines of code as follows:
+```javascript
+uint256 pointReward = blockMultiplier * pointsPerBlock * pool.allocPoint * 1e18 / totalAllocPoint / lpSupply;
+accPointsPerShare = pointReward + accPointsPerShare;
+```
+This modification ensures that the calculations are performed with the highest possible precision before any truncation occurs due to division.
diff --git a/001/019.md b/001/019.md
@@ -0,0 +1,94 @@
+Formal Charcoal Rattlesnake
+
+medium
+
+# SophonFarming: When using functions `add()` and `set()`, it should always call `massUpdatePools()` to update all pools
+
+## Summary
+
+The add() and the set() functions can only be called by the contract owner, but it is possible that the `totalAllocPoint` state will be changed without setting the `_withUpdate` parameter to true .
+
+## Vulnerability Detail
+
+The `totalAllocPoint` variable is used to determine the portion that each pool would get from the total reward, so it is one of the main factors used in the rewards calculation. Therefore, whenever the `totalAllocPoint` variable is modified without updating the pending reward first, the reward of each pool will be incorrectly calculated. For example, when `_withUpdate` is false, in the add() shown below, the `totalAllocPoint` variable will be modified without updating the rewards (massUpdatePools()).
+
+```solidity
+ function add(uint256 _allocPoint, address _lpToken, string memory _description, bool _withUpdate) public onlyOwner returns (uint256) {
+        if (poolExists[_lpToken]) {
+            revert PoolExists();
+        }
+        if (isFarmingEnded()) {
+            revert FarmingIsEnded();
+        }
+        if (_withUpdate) {
+            massUpdatePools();
+        }
+        uint256 lastRewardBlock =
+            getBlockNumber() > startBlock ? getBlockNumber() : startBlock;
+        totalAllocPoint = totalAllocPoint + _allocPoint;
+        poolExists[_lpToken] = true;
+
+        uint256 pid = poolInfo.length;
+
+        poolInfo.push(
+            PoolInfo({
+                lpToken: IERC20(_lpToken),
+                l2Farm: address(0),
+                amount: 0,
+                boostAmount: 0,
+                depositAmount: 0,
+                allocPoint: _allocPoint,
+                lastRewardBlock: lastRewardBlock,
+                accPointsPerShare: 0,
+                description: _description
+            })
+        );
+
+        emit Add(_lpToken, pid, _allocPoint);
+
+        return pid;
+    }
+```
+same as:
+```solidity
+ function set(uint256 _pid, uint256 _allocPoint, bool _withUpdate) public onlyOwner {
+        if (isFarmingEnded()) {
+            revert FarmingIsEnded();
+        }
+        if (_withUpdate) {
+            massUpdatePools();
+        }
+
+        PoolInfo storage pool = poolInfo[_pid];
+        address lpToken = address(pool.lpToken);
+        if (lpToken == address(0) || !poolExists[lpToken]) {
+            revert PoolDoesNotExist();
+        }
+        totalAllocPoint = totalAllocPoint - pool.allocPoint + _allocPoint;
+        pool.allocPoint = _allocPoint;
+
+        if (getBlockNumber() < pool.lastRewardBlock) {
+            pool.lastRewardBlock = startBlock;
+        }
+
+        emit Set(lpToken, _pid, _allocPoint);
+    }
+```
+
+## Impact
+
+Improper Reward Calculation (_withUpdate)
+
+## Code Snippet
+
+https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L153-L187
+
+https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L195-L216
+
+## Tool used
+
+Manual Review
+
+## Recommendation
+
+Removing the `_withUpdate` variable in the add() and set() functions and always calling the `massUpdatePools()` function before updating `totalAllocPoint` variable.