code-423n4 / 2023-11-shellprotocol-findings

7 stars 7 forks source link

Unwrap Fee Rounding Down: Revenue Loss, User Unfairness, and Reduced Confidence #279

Closed c4-bot-3 closed 10 months ago

c4-bot-3 commented 10 months ago

Lines of code

https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/ocean/Ocean.sol#L86-L90

Vulnerability details

Impact

The issue with the unwrap fee rounding down can have several detrimental impacts on the Ocean protocol:

  1. Revenue Loss: Due to rounding down, the contract loses out on potential unwrap fees, particularly for smaller unwrap amounts. This can significantly reduce the protocol's revenue and limit its ability to operate and maintain its services.

  2. Unfairness to Users: Smaller users who unwrap smaller amounts are disproportionately affected by the rounding down, leading to a sense of unfairness and potentially discouraging them from using the platform.

  3. Reduced User Confidence: The presence of this issue can erode user confidence in the protocol's accuracy and fairness, potentially leading to decreased user engagement and adoption.

Proof of Concept

Scenario:

This code snippet demonstrates how the integer division in the `calculateUnwrapFee` function results in rounding down the fee, leading to potential loss.

### Test:

- Deploy the Ocean smart contract with a specific unwrap fee divisor.
- Send multiple unwrap transactions with different small and large amounts of tokens.
- Verify the actual unwrap fee charged for each transaction.
- Compare the actual fees with the expected fees calculated without rounding down.
- Analyze transaction logs to identify instances of fee rounding down and quantify the potential revenue loss.
### Significant Code Traces:

- Focus on the `calculateUnwrapFee` function, specifically the line where `unwrapAmount` is divided by `unwrapFeeDivisor`. This line uses integer division, inherently causing the rounding down issue.
## Tools Used
- Solidity Compiler
- Manual Review

## Recommended Mitigation Steps
1. Implement Truncation:

Modify the `calculateUnwrapFee` function to use truncation instead of integer division.
This ensures the fee is always rounded towards zero, eliminating revenue loss for smaller unwrap amounts.
### Code:
```solidity
function calculateUnwrapFee(uint256 unwrapAmount, uint256 unwrapFeeDivisor) public pure returns (uint256) {
  return truncate(unwrapAmount / unwrapFeeDivisor);
}
  1. Set Minimum Unwrap Fee:

Define a minimum unwrap fee that applies regardless of the unwrap amount. This guarantees the contract always collects some fee, even for very small unwrap transactions.

Code:

function calculateUnwrapFee(uint256 unwrapAmount, uint256 unwrapFeeDivisor) public pure returns (uint256) {
  uint256 fee = unwrapAmount / unwrapFeeDivisor;
  if (fee < minimumUnwrapFee) {
    fee = minimumUnwrapFee;
  }
  return fee;
}
  1. Adjust Unwrap Fee Divisor:

Assessed type

Error

c4-pre-sort commented 10 months ago

raymondfam marked the issue as insufficient quality report

raymondfam commented 10 months ago

Negligible impact. Additionally, impact on 2. is supposed to be in favor of users. QA at best.

c4-pre-sort commented 10 months ago

raymondfam marked the issue as primary issue

c4-judge commented 10 months ago

0xA5DF marked the issue as unsatisfactory: Overinflated severity

0xA5DF commented 10 months ago

I agree with Raymond, this seems like intended design and negligible