Data Integrity Issue in Delegation Revocation.

[c4-submissions]

Lines of code

https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/DelegateRegistry.sol#L369-L377

Vulnerability details

Impact

A lack of checks for existing rights or if it holds any assets could lead to data inconsistency if rights or asset balances are not properly revoked or transferred during delegation revocation.

Description

The DelegateRegistry contract relies on various functions to validate permissions and ensure data integrity. However, there is a potential data inconsistency issue related to delegation revocation. When a user revokes a delegation, the _updateFrom function is called to change the status of the delegation. Still, there's no mechanism in place to check if the delegation has any existing rights or if it holds any assets (e.g., ERC20 tokens or ERC721/ERC1155 tokens). This lack of checks could lead to data inconsistency if rights or asset balances are not properly revoked or transferred during delegation revocation.

Proof of Concept

Issue Explanation

In the DelegateRegistry contract, the _updateFrom function is responsible for updating the "from" address for a delegation in storage. However, this function lacks checks to ensure that associated rights or assets are properly managed when a delegation is revoked. Here's the relevant code section: Line 369-Line 375

/// @dev Helper function that writes from whilst preserving the rest of the storage slot
function _updateFrom(bytes32 location, address from) internal {
    uint256 firstPacked = Storage.POSITIONS_FIRST_PACKED;
    uint256 cleanAddress = Storage.CLEAN_ADDRESS;
    uint256 cleanUpper12Bytes = type(uint256).max << 160;
    assembly {
        let slot := and(sload(add(location, firstPacked)), cleanUpper12Bytes)
        sstore(add(location, firstPacked), or(slot, and(from, cleanAddress)))
    }
}

The _updateFrom function changes the status of a delegation by updating the "from" address when revocation occurs. However, it does not include logic to revoke or transfer any associated rights or assets when necessary. This omission could potentially lead to data inconsistency if rights or assets are not properly managed during delegation revocation.

Potential Data Inconsistency Scenario

ERC20 Token Delegation Revocation

Scenario:

1. User A delegates the right to transfer 100 ERC20 tokens to User B using the delegateERC20 function in the DelegateRegistry contract.
2. This delegation is recorded in the contract's storage.
3. Later, User A decides to revoke the delegation using the delegateERC20 function with enable set to false.

In this scenario:

• Before Revocation:

  • Delegation exists: User A has delegated 100 ERC20 tokens to User B.
  • Delegation status: Active

• After Revocation:

  • Delegation still exists in storage.
  • Delegation status: Revoked
  • ERC20 tokens remain in User B's balance; they are not transferred back to User A.

In this situation, the issue becomes apparent. While the delegation status is correctly updated to "Revoked," the associated ERC20 tokens are not transferred back to User A. This can lead to:

• User A assuming they have regained control of the delegated tokens, leading to a discrepancy between their expectations and the actual state of their assets.
• User B retaining control of the 100 ERC20 tokens, which is inconsistent with the revoked delegation.

This inconsistency can result in confusion, disputes, or unintended consequences within the application. To address this issue and ensure data consistency, the contract should include mechanisms to transfer or revoke associated rights or assets when a delegation is revoked.

Tools Used

Manual Review

Recommended Mitigation Steps

• Asset Transfer Logic: Implement logic within the _updateFrom function to revoke or transfer any associated rights or assets when a delegation is revoked. Ensure that delegated assets are properly managed to maintain data consistency.

• Comprehensive Testing: Conduct comprehensive testing, including edge cases, to verify that assets are correctly handled during delegation revocation. This includes scenarios where rights or assets must be revoked or transferred.

Assessed type

ETH-Transfer

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid