Unrestricted Unwrap Fee Changes: Instability, Market Disruption, and Loss of Trust

[c4-bot-6]

Lines of code

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

Vulnerability details

Impact

The current changeUnwrapFee function in the Ocean smart contract allows the owner to change the unwrap fee divisor with no restrictions, leading to several negative impacts:

1. Unstable Unwrap Fees: Frequent changes in the divisor can cause instability and uncertainty for users, making it difficult to predict unwrap costs and potentially discouraging unwrapping activities.

2. Market Disruption: Sudden changes in the fee can disrupt the market for wrapped tokens, impacting liquidity and potentially causing price volatility.

3. Loss of User Trust: Unpredictable and excessive fee changes can erode user trust in the platform, leading to decreased engagement and adoption.

Proof of Concept

Scenario:

• The owner repeatedly changes the unwrap fee divisor within a short period.
• This results in significant fluctuations in the unwrap fee, making it difficult for users to plan their unwrap activities.
• The market for wrapped tokens becomes volatile due to the unpredictable fee changes, causing liquidity issues.
• Users lose trust in the platform due to the lack of transparency and control over the unwrap fees.

  Code:

  
  function changeUnwrapFee(uint256 nextUnwrapFeeDivisor) external override onlyOwner {
  if (MIN_UNWRAP_FEE_DIVISOR > nextUnwrapFeeDivisor) revert();
  emit ChangeUnwrapFee(unwrapFeeDivisor, nextUnwrapFeeDivisor, msg.sender);
  unwrapFeeDivisor = nextUnwrapFeeDivisor;
  }

### Vulnerable Test Case:
```javascript
it("should allow owner to change unwrap fee multiple times in a row", async () => {
  const initialFeeDivisor = await contract.methods.unwrapFeeDivisor().call();

  // Owner changes fee divisor multiple times
  for (let i = 0; i < 10; i++) {
    const newFeeDivisor = initialFeeDivisor + i;
    await contract.methods.changeUnwrapFee(newFeeDivisor).send({ from: ownerAddress });
  }

  // Verify that fee divisor has changed multiple times
  const finalFeeDivisor = await contract.methods.unwrapFeeDivisor().call();
  expect(finalFeeDivisor).toEqual(initialFeeDivisor + 9);
});

Logs and Significant Traces with Code:

Logs:

• Transaction logs should show the owner changing the unwrap fee divisor through the ChangeUnwrapFee event.

• These logs can be used to track changes and identify potential instability or excessive manipulation.

  Significant Traces with Code:

• The line where unwrapFeeDivisor is assigned the new value in the changeUnwrapFee function is a significant trace, highlighting the lack of restrictions on frequency or magnitude of changes.

  Tools Used

• Solidity compiler and debugger

• Manual Review

Recommended Mitigation Steps

1. Implement Cooldown Period:

• Enforce a minimum waiting period between changing the unwrap fee divisor.
• This provides stability and allows users to adjust their strategies.

  Code:

  
  uint256 public lastFeeChangeBlock;

modifier onlyAfterCooldown { require(block.number >= lastFeeChangeBlock + cooldownPeriod, "Cooldown period not completed"); _; lastFeeChangeBlock = block.number; }

function changeUnwrapFee(uint256 nextUnwrapFeeDivisor) external override onlyOwner onlyAfterCooldown { // ... }

2. Limit Fee Change Amount:

- Restrict the maximum percentage by which the unwrap fee can be changed in a single transaction.
- This prevents excessive manipulation and ensures gradual fee adjustments.
### Code:
```solidity
function changeUnwrapFee(uint256 nextUnwrapFeeDivisor) external override onlyOwner {
  require(withinChangeLimit(unwrapFeeDivisor, nextUnwrapFeeDivisor), "Fee change exceeds limit");
  // ...
}

function withinChangeLimit(uint256 currentDivisor, uint256 newDivisor) public pure returns (bool) {
  // ... implement logic to check if change is within allowed limit ...
}

Assessed type

Governance

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #182

[c4-judge]

0xA5DF marked the issue as unsatisfactory: Out of scope