Insecure and Inflexible Forwarder Approval Mechanism (Full Access Grant)

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

The current implementation of the onlyApprovedForwarder modifier in the Ocean smart contract has several negative impacts:

1. Security Risk:

Users are exposed to a significant security risk if their forwarder is compromised. An attacker can exploit full approval for all tokens and IDs to manipulate them, leading to financial losses. This can damage user trust and hinder platform adoption.

2. Lack of User Control:

Users lack fine-grained control over who can operate their tokens and IDs. They are forced to grant blanket access, limiting their ability to manage forwarder activity effectively. This can lead to unintended consequences and hinder specific use cases requiring granular control.

3. Reduced Transparency:

Users have no easy way to determine which tokens and IDs each forwarder is authorized to operate on. This hinders monitoring and managing forwarder activity, making it difficult to detect unauthorized actions. Lack of transparency can erode user trust and confidence in the platform.

Proof of Concept

Scenario:

• User grants full approval for all tokens and IDs to a forwarder.
• Forwarder gets compromised by an attacker.
• Attacker uses the compromised forwarder to transfer all user tokens and IDs to a different account.

  Code:

  
  modifier onlyApprovedForwarder(address userAddress) {
  if (!isApprovedForAll(userAddress, msg.sender)) revert FORWARDER_NOT_APPROVED();
  _;
  }

This code snippet demonstrates how the `onlyApprovedForwarder` modifier relies on the `isApprovedForAll` function for approval checks, leading to the security and control issues discussed above.

### Vulnerable Test Case:
```javascript
it("should allow compromised forwarder to transfer tokens", async () => {
  // Grant full approval to forwarder
  await contract.methods.setApprovalForAll(forwarderAddress, true).send({ from: userAddress });

  // Compromise forwarder and transfer tokens
  await forwarderContract.methods.transferFrom(userAddress, attackerAddress, tokenId).send({ from: attackerAddress });

  // Verify that tokens are transferred
  const userBalance = await contract.methods.balanceOf(userAddress).call();
  expect(userBalance).toEqual(0);

  const attackerBalance = await contract.methods.balanceOf(attackerAddress).call();
  expect(attackerBalance).toEqual(tokenId);
});

This test case simulates how a compromised forwarder can transfer tokens from a user's account after receiving full approval.

Logs and Significant Traces with Code:

Logs:

• Transaction logs should show the approval granted by the user to the forwarder.

• They should also show the transfer of tokens from the user's account to the attacker's account.

• These logs can be used to identify unauthorized activities and investigate potential security breaches.

  Significant Traces with Code:

• Focus on the setApprovalForAll function and its interaction with the isApprovedForAll function.

• This highlights the granting of full approval and its potential security implications. Also, analyze the transferFrom function executed by the compromised forwarder to - understand how unauthorized token transfers occur.

  Tools Used

• Solidity compiler

• Manual

  Recommended Mitigation Steps

  1. Implement Granular Approval:

• Develop separate approval functions for specific tokens or IDs.

• This allows users to grant targeted access to forwarders for specific operations.

  Code:

  
  function approveTokenForForwarder(address forwarder, address tokenAddress) public {
  // ... Grant approval for specific token ...
  }

function approveIdForForwarder(address forwarder, uint256 id) public { // ... Grant approval for specific token ID ... }

2. Introduce Role-Based Access Control:

- Define roles for forwarders with specific permissions and limitations.
- This provides a more robust and flexible way to control forwarder activity.
### Code:
```solidity
enum ForwarderRole {
  Transfer,
  Burn,
  Mint
}

function assignForwarderRole(address forwarder, ForwarderRole role) public {
  // ... Assign specific role to forwarder ...
}

3. Enhance Transparency:

• Implement mechanisms for users to easily view and manage forwarder approvals for different tokens, IDs, and roles.
• This allows for better awareness and control over delegated access.

4. Implement additional security measures:

• Consider utilizing multi-signature wallets or time-lock mechanisms for sensitive transactions involving forwarders.

This adds an extra layer of security and reduces the risk of unauthorized activities. By implementing these recommended mitigation steps, the smart contract can address the issues related to the onlyApprovedForwarder modifier and establish a more secure, flexible, and transparent forwarder approval system.

Assessed type

Access Control

[raymondfam]

Already warned as commented:

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

     * Because poorly chosen interactions are vulnerable to economic attacks,
     *  calling do{Interaction|MultipleInteractions} on a user's behalf must
     *  require the  same level of trust as direct balance transfers.

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-judge]

0xA5DF marked the issue as unsatisfactory: Overinflated severity

[0xA5DF]

Seems like a known issue/intended design