ENS - Limbooo's results

QA Report

Summary

Total 9 instances over 7 issues:

L: Low impact issue, N: Non Critical Issues

---

[L-01] Delegating to Address Zero

The contract allows users to delegate to address zero (0x0), which effectively locks up tokens without a valid delegate. This can be achieved when one of the entities in the targets array in the delegateMulti params has a value of 0.

Recommendation: Add checks to prevent delegating to address zero or implement specific actions.

• ERC20MultiDelegate.sol#L59

[L-02] Token Transfer to ERC20ProxyDelegator

Transferring tokens directly to an ERC20ProxyDelegator contract before or after the creation is currently counted as a valid vote but is not recoverable. This could lead to unintentional loss of tokens.

Recommendation: Implement a mechanism to recover tokens sent directly to ERC20ProxyDelegator for improved user experience.

• ERC20MultiDelegate.sol#L11-#L20

[N-03] No Standardized Data Format for delegateMulti

The delegateMulti function takes three arrays (sources, targets, and amounts) as arguments, making it less user-friendly and challenging to integrate with other contracts and tools.

Recommendation: Consider implementing a structured data format for better usability.

Code Snippet:

struct Delegation {    
    // maybe type and owner
    address source;
    address target;
    uint256 amount;
}

function delegateMulti(Delegation[] calldata delegations) external {
    // Process delegations using the structured format.
    for (uint256 i = 0; i < delegations.length; i++) {
        _processDelegation(delegations[i]);
    }
    //...
}

• ERC20MultiDelegate.sol#L57-#L61

[L-04] Lack of a Pausable Mechanism

The contract does not include a pausable mechanism, which can be essential for emergency scenarios, bug fixes, or temporary halting of certain functions.

Recommendation: Implement a pausable mechanism for added security and control.

• ERC20MultiDelegate.sol#L124

• ERC20MultiDelegate.sol#L155

[L-05] Interaction with Approved Operators in ERC1155

Description: The contract does not allow approved operators in ERC1155 to implement the delegateMulti function on behalf of their users. This limitation can impact usability.

Recommendation: Explore options to allow approved operators to interact with delegateMulti.

• ERC20MultiDelegate.sol#L57

[N-06] Same Source and Target

When the source and target addresses are the same, the contract still executes a transfer operation, but this doesn't have any practical impact on the contract's functionality. It results in the emission of redundant events.

Recommendation: Optimize the code to avoid redundant event emissions in this scenario.

• ERC20MultiDelegate.sol#L134

[N-07] Lack of Event Emission

Token transfers from a source delegate (proxy delegator) back to the user in the _reimburse function and during the creation of proxy delegators are missing event emissions. This absence of event emission may lead to confusion and misunderstandings about the state of the current situation, as users are not notified when these functionality occur.

Recommendation: Implement an event emission mechanism to notify users whenever tokens are transferred during the reimbursement process in the _reimburse function and when tokens are sent to proxy delegators. This will improve transparency and user experience.

Instances:

• ERC20MultiDelegate.sol#L144-#L149 - Lack of event emission during token transfers in the _reimburse function.
• ERC20MultiDelegate.sol#L55-#L161 - Lack of event emission during transfer to proxy delegators.

[N-08] Redundant Balance Check in _processDelegation

In the _processDelegation function, there's a redundant balance check on the user's account, ensuring they have a sufficient balance for the token transfer. However, the standard balanceOf function check is already performed within the _burnBatch function, which is part of the ERC1155 standard behavior.

Recommendation: Remove the redundant balance check within _processDelegation to optimize gas costs and improve code efficiency.

• ERC20MultiDelegate.sol#L81-#L83

Analysis Report

Table of Contents

• Introduction
  • Background
  • Objective
• Approach Taken and Scope
  • Tested scenario
• Architecture
  • validating
  • Creation
  • Limitation
  • Architecture Recommendations
• Codebase Quality
• Centralization Risks
• Mechanism Review
• Systemic Risks
• Other Considerations
• Summary

Introduction

Background

This code snippet is part of a utility contract that allows delegators to pick multiple delegates for an ERC20 token. It involves complex interactions with ERC20Votes and ERC1155 standards. The code aims to improve the delegation process for token holders.

Objective

The analysis below highlights critical aspects and potential improvements.

Approach Taken and Scope

The evaluation involves a line-by-line analysis of the code, considering best practices, standards, and potential issues. The focus is on code quality, security, and usability with relevant contracts from OpenZeppelin lib and some ENS implementation contracts.

Tested scenario

Some testing was established to validate a variant attack scenarios as follow but not limited to:

• Reentarcy over delegateMulti,

• ERC20 transfer over ERC20ProxyDelegator before and after creation,

• Trying to build complex duplicate sources and targets as delegate batches,

• Multilevel delegating over a deployed proxy delegators.

Architecture

validating

Excellency approach was used to validate the msg.sender balances by relaying on ERC1155 implementation upon burning and minting, some of execution like when transferring between proxy delegators it check sender balance of the source proxy delegator while it will be prevented when ERC1155 burn batch executed.

Creation

The contract deploys proxy delegators for each delegate address when needed. It follows a standard pattern for deploying a contract with a deterministic address based on the contract's creation code and other parameters. The code ensures that a proxy delegator contract is created for each unique combination of _token and _delegate without the risk of deploying duplicate contracts with the same combination of arguments.

Limitation

While there is no risks found here, but the code dose not limit user interactions like delegating to target zero, or transferring between duplicate addresses. If an attacker provides duplicate entries in the sources or targets arrays, the contract will process transfers for the same amount of tokens as specified in the amounts array. The contract won't create extra tokens, and if the source delegate doesn't have enough tokens to cover the specified amount, the transaction will revert. Even though, the attacker will not receive extra tokens, but the transfers may still occur as intended, or they may revert due to insufficient funds in the source delegate's balance. However The key concern in such cases is more about the potential for confusion and unintended behavior.

Architecture Recommendations

1. Pausable Mechanism: Implement a pausable mechanism to allow emergency halting of specific functions for added security and control. Pausability is crucial in smart contracts.

2. User Experience Enhancements: Improve the user experience by implementing additional user notifications or feedback mechanisms to keep users informed about the status of their delegations or token transfers.

3. ERC1155 Approved Operators: Consider implementing support for ERC1155 approved operators. This feature allows token holders to authorize specific addresses as operators to perform token delegation on their behalf. This enhancement can simplify delegation and provide more control to token holders, enhancing the user experience.

4. Consistency with ERC Standards: Ensure that the codebase remains consistent with the latest versions of relevant ERC standards, such as ERC20, ERC1155, and ERC20Votes.

Codebase Quality

• The code structure is well-organized, following OpenZeppelin standards.

• Function and variable names are descriptive, enhancing readability.

• It uses OpenZeppelin libraries and adheres to recognized standards. Updating to latest version will enhance security and gas usage.

• Code duplication for event emission can be reduced by optimizing event emission in specific scenarios, such as same-source-and-target transfers.

Centralization Risks

• The code's primary risk is centralization in the creation and operation of proxy delegators. Centralization can lead to dependencies on specific entities for the creation and operation of these delegators. Decentralization efforts should be considered.

Mechanism Review

• The primary mechanism involves creating proxy delegator contracts, transferring tokens to them, and processing delegation transfers. This mechanism simplifies delegation for token holders.

Systemic Risks

• The code's primary systemic risk is the potential loss of tokens when transferred directly to an ERC20ProxyDelegator without recovery mechanisms. Users need to be cautious when interacting with proxy delegators.

Other Considerations

• Ensure that users are aware of the potential for losing tokens when transferring them directly to ERC20ProxyDelegators.

• Enhance user experience by adding mechanisms to recover tokens sent directly to ERC20ProxyDelegators, maybe fixing this will lead to inheriting ERC1155Supply.

Summary

The ERC20MultiDelegate code is well-structured and adheres to recognized standards. It simplifies delegation for token holders but comes with centralization and token loss risks. Implementing a pausable mechanism and optimizing event emission can enhance the codebase.

Time spent:

16 hours