Uploaded files for judging

sherlock-audit · Aug 29, 2024 · e500316 · e500316
1 parent 6fefffa
commit e500316
Show file tree

Hide file tree

Showing 190 changed files with 10,946 additions and 10 deletions.
diff --git a/.gitignore b/.gitignore
diff --git a/001/001.md b/001/001.md
@@ -0,0 +1,74 @@
+Soft Holographic Mule
+
+High
+
+# Users will bypass redemption fees in `PointTokenVault`
+
+### Summary
+
+Incorrect updating of `feelesslyRedeemedPTokens` will cause a loss of revenue for the protocol as users will exploit the [`redeemRewards` function](https://github.com/sherlock-audit/2024-07-sense-points-marketplace/blob/main/point-tokenization-vault/contracts/PointTokenVault.sol#L172-L226) to avoid paying fees on subsequent redemptions.
+
+
+### Root Cause
+
+In [point-tokenization-vault/contracts/PointTokenVault.sol:redeemRewards()](https://github.com/sherlock-audit/2024-07-sense-points-marketplace/blob/main/point-tokenization-vault/contracts/PointTokenVault.sol#L172-L226) the `feelesslyRedeemedPTokens` value is not correctly updated when partial fees are applied.
+
+
+### Internal pre-conditions
+
+1. User needs to claim PTokens to set `claimedPTokens[user][pointsId]` to be greater than 0.
+2. `redemptionFee` needs to be set to a value greater than 0.
+
+
+### External pre-conditions
+
+None
+
+### Attack Path
+
+1. User claims a large amount of PTokens, e.g., 1,000,000.
+2. User redeems a small portion of PTokens, e.g., 1 PToken, which is feeless.
+3. feelesslyRedeemedPTokens[user][pointsId] is updated to 1.
+4. User redeems the remaining 999,999 PTokens.
+5. Since feelesslyRedeemable (999,999) >= pTokensToBurn (999,999), no fee is charged.
+6. Steps 4-5 can be repeated for future claims, always avoiding fees.
+
+
+### Impact
+
+The protocol suffers a loss of expected redemption fees. Users can bypass fees on the majority of their redemptions, potentially leading to significant revenue loss for the protocol.
+
+### PoC
+
+
+Consider the following scenario:
+
+1. Alice claims 1,000,000 PTokens for a specific `pointsId.` This sets her `claimedPTokens[Alice][pointsId]` to 1,000,000.
+
+2. Alice decides to redeem 1 PToken:
+   - She calls `redeemRewards()` with `amountToClaim = 1`.
+   - Since this is her first redemption, it's feeless (`feelesslyRedeemable >= pTokensToBurn`).
+   - The function updates `feelesslyRedeemedPTokens[Alice][pointsId]` to 1.
+
+3. Alice then redeems the remaining 999,999 PTokens:
+   - She calls `redeemRewards()` again with `amountToClaim = 999,999`.
+   - The function calculates `feelesslyRedeemable = claimedPTokens - feelesslyRedeemedPTokens = 1,000,000 - 1 = 999,999`.
+   - Since `feelesslyRedeemable (999,999) >= pTokensToBurn (999,999)`, the redemption is considered feeless.
+   - No fee is charged, and Alice receives the full amount of reward tokens.
+
+4. The issue lies in the `else` block of the `redeemRewards()` function:
+   ```solidity
+   if (feelesslyRedeemed != claimed) {
+       feelesslyRedeemedPTokens[msg.sender][pointsId] = claimed;
+   }
+   ```
+   This condition is never met in our scenario, so `feelesslyRedeemedPTokens` is not updated correctly.
+
+5. As a result, Alice has redeemed all her PTokens without paying any fees, despite the protocol intending to charge a fee on the majority of her redemption.
+6. Alice can repeat this process for future claims, always avoiding fees on the bulk of her redemptions.
+
+
+
+### Mitigation
+
+To fix this issue, update the `feelesslyRedeemedPTokens` value correctly in all cases within the [`redeemRewards` function](https://github.com/sherlock-audit/2024-07-sense-points-marketplace/blob/main/point-tokenization-vault/contracts/PointTokenVault.sol#L172-L226)
diff --git a/001/007.md b/001/007.md
@@ -0,0 +1,51 @@
+Suave Yellow Horse
+
+High
+
+# Users may lose their pToken rewards forever due to inaccurate claiming access control of the claimPToken function
+
+## Summary
+Logically and normally, when the `entruster` entrustes an `entrustee`, it is assumed that the `claimPToken` function is:
+1. Either called by the `entrustee` **on behalf** of the `entruster`, and the claimed tokens are sent to the **`entruster` (as in many automation solutions)** (the `entrustee` specifies the `recipient=entruster`),
+2. Or called by the `entrustee` **on behalf** of the `entruster`, but the `recipient` is set to any address that the `entrustee` wants,
+3. Or called by the `entruster` himself, and the recipient can be anyone.
+
+## Vulnerability Detail
+Consider a scenario where Alice entrustes an AutomationBot to call `claimPToken` for her once in a while.
+
+The AutomationBot is a fair actor: he will only call `claimPToken` with `_account=Alice` and `_receiver=Alice`.
+
+The `AutomationBot` can call `claimPTokens` for Alice, but he can never call `redeemRewards` for his own address.
+
+However, Bob comes in and he wants to permanently freeze Alice's rewards.
+
+Bob will call `claimPToken` and set `_account=Alice` and **`receiver=AutomationBot`**.
+
+### Assume the AutomationBot doesn't have any funds receiving functionality: Alice will never get her pToken reward back, despite the fairness of AutomationBot.
+The AutomationBot doesn't implement any functionality that would allow Alice to force him to burn transfer the minted pToken back to her.
+
+**In that case, the AutomationBot is by definition an entrusted actor of Alice, so, logically, the comparison that would prevent permanently freezing Alice's funds should be passing only if the conditions `msg.sender == _account || msg.sender == trustedClaimers[_account][msg.sender]` resolves to true.**
+
+## Impact
+Anyone can permanently skim Alice's rewards to her `entrustee`, without any entruster's or entrustee's consent for that, by just calling `claimPTokens` at an appropriate time.
+
+## Code Snippet
+```solidity
+        if (_account != _receiver && !trustedClaimers[_account][_receiver]) {
+            revert NotTrustedClaimer();
+        }
+```
+
+https://github.com/sherlock-audit/2024-07-sense-points-marketplace/blob/076bf833f4dc1418e93c8172e4a4110344f1c812/point-tokenization-vault/contracts/PointTokenVault.sol#L152-L154
+
+## Tool used
+Mnaual human review.
+
+## Recommendation
+To handle claiming approvals functionality for cases when the entrustee calls the `claimPTokens` function on behalf of the entruster, consider checking that the `msg.sender == trustedClaimers[_account][_receiver]` instead of ignoring the `msg.sender` and checking that the `_receiver` exists in the `trustedClaimers[_account]`.
+
+Such as tailoring the current code of the `PointTokenVault` as follows:
+```diff
+-         if (_account != _receiver && !trustedClaimers[_account][_receiver]) {
++         if (_account != _receiver && !trustedClaimers[_account][msg.sender]) {
+```
diff --git a/001/010.md b/001/010.md
@@ -0,0 +1,88 @@
+Sparkly Burgundy Wombat
+
+Medium
+
+# Attacker can disable the `trustClaimers`.
+
+## Summary
+Vault allows `receiver`(`trustedClaimers`) to claim `PTokens` on behalf of `account`.
+Attacker can front-run `receiver`'s claim tx and can always trasfer `PTokens` to original `account` not to `receiver`(`trustedClaimers`).
+## Vulnerability Detail
+The relevant code of `PointTokenVault.claimPTokens()` is following.
+```solidity
+    function claimPTokens(Claim calldata _claim, address _account, address _receiver) public {
+        bytes32 pointsId = _claim.pointsId;
+
+        bytes32 claimHash = keccak256(abi.encodePacked(_account, pointsId, _claim.totalClaimable));
+        _verifyClaimAndUpdateClaimed(_claim, claimHash, _account, claimedPTokens);
+
+        if (address(pTokens[pointsId]) == address(0)) {
+            revert PTokenNotDeployed();
+        }
+
+152     if (_account != _receiver && !trustedClaimers[_account][_receiver]) {
+            revert NotTrustedClaimer();
+        }
+
+        uint256 pTokenFee = FixedPointMathLib.mulWadUp(_claim.amountToClaim, mintFee);
+        pTokenFeeAcc[pointsId] += pTokenFee;
+
+        pTokens[pointsId].mint(_receiver, _claim.amountToClaim - pTokenFee); // Subtract mint fee.
+
+        emit PTokensClaimed(_account, _receiver, pointsId, _claim.amountToClaim, pTokenFee);
+    }
+```
+If `_receiver` differs with `_account`, `_receiver` should be trusted beforehand by the following `trustClaimer()`.
+```solidity
+    function trustClaimer(address _account, bool _isTrusted) public {
+        trustedClaimers[msg.sender][_account] = _isTrusted;
+        emit TrustClaimer(msg.sender, _account, _isTrusted);
+    }
+```
+Then the following attack path is available.
+1. `_account` trusts `_receiver` by calling `trustClaimer(_receiver, true)`.
+2. `_receiver` calls `claimPTokens(claim, _account, _receiver)` to receive `PTokens`.
+3. Attacker front-runs `_receiver`'s claim tx by calling `claimPTokens(claim, _account, _account)` to transfers `PTokens` to `_account`.
+4. `_receiver`'s tx will be reverted because all `PTokens` are already transferred to `_account`.
+5. As a result, attacker can prevent `_receiver` from transferring `PTokens` to `_receiver`.
+
+## Impact
+Attacker can prevent `_receiver` from transferring `PTokens` to `_receiver` or original `_account` from transferring `PTokens` to himself. That is, attacker can disable the core function of contract and damage the reputation of the protocol.
+
+In general, this issue doesn't cause loss of funds because the `PTokens` are transferred to original account and can be transferred again to `receiver` manually. However, if `_account` is a smart contract and the contract has no function to transfer `PTokens`, the tokens may be locked there.
+
+## Code Snippet
+- [PointTokenVault.claimPTokens()](https://github.com/sherlock-audit/2024-07-sense-points-marketplace/blob/main/point-tokenization-vault/contracts/PointTokenVault.sol#L142-L162)
+
+## Tool used
+
+Manual Review
+
+## Recommendation
+It is recommeded to add authority check in `PointTokenVault.claimPTokens()`.
+```diff
+    function claimPTokens(Claim calldata _claim, address _account, address _receiver) public {
+        bytes32 pointsId = _claim.pointsId;
+
+        bytes32 claimHash = keccak256(abi.encodePacked(_account, pointsId, _claim.totalClaimable));
+        _verifyClaimAndUpdateClaimed(_claim, claimHash, _account, claimedPTokens);
+
+        if (address(pTokens[pointsId]) == address(0)) {
+            revert PTokenNotDeployed();
+        }
+
++       if (msg.sender != _account && msg.sender != _receiver) {
++           revert;
++       }
+        if (_account != _receiver && !trustedClaimers[_account][_receiver]) {
+            revert NotTrustedClaimer();
+        }
+
+        uint256 pTokenFee = FixedPointMathLib.mulWadUp(_claim.amountToClaim, mintFee);
+        pTokenFeeAcc[pointsId] += pTokenFee;
+
+        pTokens[pointsId].mint(_receiver, _claim.amountToClaim - pTokenFee); // Subtract mint fee.
+
+        emit PTokensClaimed(_account, _receiver, pointsId, _claim.amountToClaim, pTokenFee);
+    }
+```
diff --git a/001/019.md b/001/019.md
@@ -0,0 +1,148 @@
+Quaint Tangelo Raccoon
+
+Medium
+
+# Fee collector should pay `rewardTokenFee` when redeem rewards for `pTokenFee`.
+
+## Summary
+Vault has two kinds of fees: `pTokenFee` and `rewardTokenFee`.
+Due to some errors, `feeCollector` should pay `rewardTokenFee` while redeem rewards for `pTokenFee`, but users can avoid `rewardTokenFee` for some `PTokens`.
+
+## Vulnerability Detail
+`PointTokenVault.claimPTokens()` function is following.
+```solidity
+    function claimPTokens(Claim calldata _claim, address _account, address _receiver) public {
+        bytes32 pointsId = _claim.pointsId;
+
+        bytes32 claimHash = keccak256(abi.encodePacked(_account, pointsId, _claim.totalClaimable));
+146:    _verifyClaimAndUpdateClaimed(_claim, claimHash, _account, claimedPTokens);
+
+        if (address(pTokens[pointsId]) == address(0)) {
+            revert PTokenNotDeployed();
+        }
+
+        if (_account != _receiver && !trustedClaimers[_account][_receiver]) {
+            revert NotTrustedClaimer();
+        }
+
+        uint256 pTokenFee = FixedPointMathLib.mulWadUp(_claim.amountToClaim, mintFee);
+        pTokenFeeAcc[pointsId] += pTokenFee;
+
+159:    pTokens[pointsId].mint(_receiver, _claim.amountToClaim - pTokenFee); // Subtract mint fee.
+
+        emit PTokensClaimed(_account, _receiver, pointsId, _claim.amountToClaim, pTokenFee);
+    }
+```
+Assume that `_account == _receiver`.
+`_verifyClaimAndUpdateClaimed()` of `L146` is following.
+```solidity
+    function _verifyClaimAndUpdateClaimed(
+        Claim calldata _claim,
+        bytes32 _claimHash,
+        address _account,
+        mapping(address => mapping(bytes32 => uint256)) storage _claimed
+    ) internal {
+        --- SKIP ---
+        // Update the total claimed amount.
+        unchecked {
+294:        _claimed[_account][pointsId] = alreadyClaimed + amountToClaim;
+        }
+    }
+```
+As can be seen, the claimer (`_account`) get `PTokens` of `amountToClaim - pTokenFee` on `L159` but `claimedPTokens[_account][pointsId]` is increased by `amountToClaim`.
+On the other hand, `collectFees()` function is following.
+```solidity
+    function collectFees(bytes32 _pointsId) external {
+        (uint256 pTokenFee, uint256 rewardTokenFee) = (pTokenFeeAcc[_pointsId], rewardTokenFeeAcc[_pointsId]);
+
+        if (pTokenFee > 0) {
+347:        pTokens[_pointsId].mint(feeCollector, pTokenFee);
+            pTokenFeeAcc[_pointsId] = 0;
+        }
+
+        if (rewardTokenFee > 0) {
+            // There will only be a positive rewardTokenFee if there are reward tokens in this contract available for transfer.
+            redemptions[_pointsId].rewardToken.safeTransfer(feeCollector, rewardTokenFee);
+            rewardTokenFeeAcc[_pointsId] = 0;
+        }
+
+        emit FeesCollected(_pointsId, feeCollector, pTokenFee, rewardTokenFee);
+    }
+```
+The `feeCollector` get `pTokenFee` `PTokens` on `L347` but `claimedPTokens[feeCollector][pointsId]` is not increased.
+Users can avoid `rewardTokenFee` up to `claimedPTokens` variable in the following `redeemRewards()` function.
+```solidity
+    function redeemRewards(Claim calldata _claim, address _receiver) public {
+        --- SKIP ---
+        uint256 claimed = claimedPTokens[msg.sender][pointsId];
+        uint256 feelesslyRedeemed = feelesslyRedeemedPTokens[msg.sender][pointsId];
+
+        // The amount of pTokens that are free to redeem without fee.
+        uint256 feelesslyRedeemable = claimed - feelesslyRedeemed;
+        --- SKIP ---
+    }
+```
+Therefore, the following scenario is available:
+1. Assume that `user1` claimed `1000` `PTokens` and `mintFee` is `10%`.
+2. `user1` pays `100` `PTokens` to `feeCollector` and receives only `900` `PTokens` but `claimedPTokens[_account][pointsId]` variable increases by `1000`.
+3. `user1` redeem his own `900` `PTokens` feelessly. Not only that, `user1` can buy `100` more `PTokens` from others and redeem them too feelessly.
+4. On the other hand, `feeCollector` has to pay `rewardTokenFee` when he redeem his own `100` `PTokens`.
+
+## Impact
+`feeCollector` has to pay `rewardTokenFee` when he redeem his own `pTokenFee` and it makes the fee calculation incorrect.
+Users can avoid `rewardTokenFee` for the `pTokenFee` `PTokens`.
+
+## Code Snippet
+https://github.com/sherlock-audit/2024-07-sense-points-marketplace/blob/main/point-tokenization-vault/contracts/PointTokenVault.sol#L142-L162
+https://github.com/sherlock-audit/2024-07-sense-points-marketplace/blob/main/point-tokenization-vault/contracts/PointTokenVault.sol#L343-L358
+
+## Tool used
+
+Manual Review
+
+## Recommendation
+Modify `PointTokenVault.claimPTokens()` function as follows.
+```solidity
+    function claimPTokens(Claim calldata _claim, address _account, address _receiver) public {
+        bytes32 pointsId = _claim.pointsId;
+
+        bytes32 claimHash = keccak256(abi.encodePacked(_account, pointsId, _claim.totalClaimable));
+        _verifyClaimAndUpdateClaimed(_claim, claimHash, _account, claimedPTokens);
+
+        if (address(pTokens[pointsId]) == address(0)) {
+            revert PTokenNotDeployed();
+        }
+
+        if (_account != _receiver && !trustedClaimers[_account][_receiver]) {
+            revert NotTrustedClaimer();
+        }
+
+        uint256 pTokenFee = FixedPointMathLib.mulWadUp(_claim.amountToClaim, mintFee);
+        pTokenFeeAcc[pointsId] += pTokenFee;
+++      claimedPTokens[_account][pointsId] -= pTokenFee;
+
+        pTokens[pointsId].mint(_receiver, _claim.amountToClaim - pTokenFee); // Subtract mint fee.
+
+        emit PTokensClaimed(_account, _receiver, pointsId, _claim.amountToClaim, pTokenFee);
+    }
+```
+And modify `PointTokenVault.collectFees()` function as follows.
+```solidity
+    function collectFees(bytes32 _pointsId) external {
+        (uint256 pTokenFee, uint256 rewardTokenFee) = (pTokenFeeAcc[_pointsId], rewardTokenFeeAcc[_pointsId]);
+
+        if (pTokenFee > 0) {
+            pTokens[_pointsId].mint(feeCollector, pTokenFee);
+++          claimedPTokens[feeCollector][_pointsId] += pTokenFee;
+            pTokenFeeAcc[_pointsId] = 0;
+        }
+
+        if (rewardTokenFee > 0) {
+            // There will only be a positive rewardTokenFee if there are reward tokens in this contract available for transfer.
+            redemptions[_pointsId].rewardToken.safeTransfer(feeCollector, rewardTokenFee);
+            rewardTokenFeeAcc[_pointsId] = 0;
+        }
+
+        emit FeesCollected(_pointsId, feeCollector, pTokenFee, rewardTokenFee);
+    }
+```