ENS - 0x3b's results

[L-01] Possible re-entrancy in _delegateMulti

There is a re-entracy in _delegateMulti where the attack contract is called by ERC20MultiDelegate. I didn't have enough time to manage the exploit, because of it I am putting it as low, as it's not a finished exploit.

<details> <summary>PoC</summary>

// SPDX-License-Identifier: MIT
pragma solidity 0.8.19;

import {Test} from "forge-std/Test.sol";
import {console} from "forge-std/console.sol";
import {ERC20ProxyDelegator,ERC20MultiDelegate} from "../contracts/ERC20MultiDelegate.sol";
import {MyVotes} from "../contracts/MyVotes.sol";
import {IERC1155Receiver} from "@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol";

contract MultiTest is Test{
    address attackerAddress;
    ERC20MultiDelegate multi;
    ERC20ProxyDelegator delegator;
    MyVotes token;
    Attacker2 attacker2;

    function setUp() public {
        vm.label(attackerAddress,"Attaker");
        token = new MyVotes(address(111));
        multi = new ERC20MultiDelegate(token, "idk mate");
        attacker2 = new Attacker2(token,address(multi));
        attackerAddress = address(attacker2);
        token.mint(attackerAddress,1000e18);
    }

    function test_reEnter() public {
        uint256[] memory sources = new uint[](0);
        uint256[] memory targets = new uint[](1);
        uint256[] memory amounts = new uint[](1);
        
        {
            targets[0] = uint256(uint160(attackerAddress));
            amounts[0] = 1000e18;
        }
        vm.startPrank(attackerAddress);
        token.approve(address(multi), 1e24);
        multi.delegateMulti(sources, targets, amounts);
        vm.stopPrank();
    }
}
contract Attacker2{
    MyVotes token;
    ERC20MultiDelegate multi;
    constructor(MyVotes _token,address _multi) {
        token = MyVotes(_token);
        token.approve(_multi,type(uint256).max);
        multi = ERC20MultiDelegate(_multi);
    }
    function onERC1155Received(address, address, uint256, uint256, bytes memory) public virtual returns (bytes4) {
        console.log("onERC1155Received re-entered");
        return this.onERC1155Received.selector;
    }
    function onERC1155BatchReceived(address, address, uint256[] memory, uint256[] memory, bytes memory) public virtual returns (bytes4) {
        console.log("onERC1155BatchReceived re-entered");
        return this.onERC1155BatchReceived.selector;
    }

    function onERC721Received(address, address, uint256, bytes memory) public virtual returns (bytes4) {
        console.log("onERC721Received re-entered");
        return this.onERC721Received.selector;
    }
}

Logs:
  onERC1155Received re-entered

</details>

[L-02] The whole contract does not work with approvals

The whole ERC20MultiDelegate does not work with approvals. This means that is a user approves another user for his 1155 tokens, the other user will no be able to use them with ERC20MultiDelegate.

Example:

1. User1 has 200 tokens with id 333 and 200 tokens with id 111.
2. User2 has 200 tokens with id 333.
3. User2 approves User1 for his 200 tokens with id 333.
4. User1 call delegateMulti with params for _processDelegation to swap the 200 tokens (id: 333) to 200 (id: 111).

However due to the way _processDelegation makes the swap with the inside ERC20Vote tokens, user1 swaps his 200 (id: 333) tokens instead of user2's.

    function _processDelegation(address source,address target,uint256 amount) internal {
        uint256 balance = getBalanceForDelegate(source);
        assert(amount <= balance);

        deployProxyDelegatorIfNeeded(target);
        transferBetweenDelegators(source, target, amount);

        emit DelegationProcessed(source, target, amount);
    }

    function transferBetweenDelegators(address from,address to,uint256 amount) internal {
        address proxyAddressFrom = retrieveProxyContractAddress(token, from);
        address proxyAddressTo = retrieveProxyContractAddress(token, to);
        token.transferFrom(proxyAddressFrom, proxyAddressTo, amount);
    }

5. User1 has 400 tokens (id: 111) and 0 (id: 333).
6. User2 still has his 200 (id: 333) tokens.

I would suggest to implement a way for approvals to work with the current contract.

[L-03] Quadratic voting is less secure with ERC20ProxyDelegator

The power of quadratic voting is that large volumes of vote tokens will be square rooted. However with the help of ERC20ProxyDelegator and it's ability to instantly delegate to 100+ addresses sibyl attacks on quadratic voting systems become more possible and with time more common.

If attacker votes with 10k tokens with a single address => 100 votes. However he can simply split 10k tokens between 100 addresses and vote with 100 tokens per address (10 votes) => 1000 votes. Applying 10x to his voting power.

Because this issues is about the whole structure of the system, I am not able to give a guarantied fix. Will leave it for the devs to decide.

[L-04] If a user claims to his ERC20ProxyDelegator his tokens will be lost

When users claim from ENSToken they specifically need to claim to themselves and not to their own ProxyDelegator. This is because claimTokens transfers the tokens directly to the msg.sender and when transferred to ProxyDelegator, no ERC1155 will be minted to the user.

Example:

1. User1 claims his ENS tokens to himself, and then uses MultiDelegate to spread them, in return receiving ERC1155
2. User2 claims his ENS tokens directly to his ProxyDelegator

User2 will not receive any ERC1155 tokens and the vote tokens will be stuck forever in his ProxyDelegator. He will be able to vote with them, however if he want to delegate them to someone else, he will not be able to do so.

[L-05] The wrapper will not work with any token

The sponsor explained that the wrapper - ERC20MultiDelegate can be used with any vote token and it's not only for ENS to use it. However there is an issue as some vote tokens disable transfers. Usually this is done in order for the DAO to have a better time tracking votes, and avoids many vulnerabilities (multiple votings, vote->transfer->vote, ...).

Because they disable the transfer and ERC20MultiDelegate works directly by transferring these vote tokens to another contract (ProxyDelegator) the whole contract will not function for such tokens. This is not a massive issue, and for now I am unaware of how it can be fixed (don't make transfers, yet delegate to multiple users), hence I am putting it as a low.

[N-01] If a user send any vote tokens to ERC20ProxyDelegator they will be lost forever

If a user sends any VoteTokens to ERC20ProxyDelegator they will be counted as votes, since this contract delegates to someone, however they will be stuck there forever.

Description

ERC20MultiDelegate is a combination of two smart contracts:

• ERC20ProxyDelegator - This is a basic contract used only to store the voting tokens and delegate them to a delegate. This contract is created for every user who uses ERC20MultiDelegate to vote or acquire voting tokens.

• ERC20MultiDelegate - This is the more advanced contract that manages delegations. With it, users can delegate their voting tokens to other users, or, if they have already delegated, change the delegation, or withdraw the voting tokens.

This combination of contracts makes it possible for users to store their delegated tokens in another contract and exchange and trade their representatives, which are ERC1155 tokens.

Findings

The codebase was really simple and straightforward. As I expected, there were not many bugs, or at least I didn't encounter anything major. The report is filled with lows and non-critical bugs. Some time was spent exploring some out-of-scope codebases to gather reference points, although there were not many contracts that implemented ERC20MultiDelegate. I was interested in the DAO mechanism used with these voting tokens; however, I was not able to find it. Probably, it is still under development. Approximately 80% of my time was dedicated to attempting to manipulate the rewards, users' vote balances, transfers, and approvals. I wrote multiple tests (~600 lines in Foundry), and the code turned out more secure than I had hoped ):

Codebase Quality

The code quality was excellent, featuring simple yet proficient code. However, due to numerous missing pieces of information related to how these vote tokens will be used with other systems, I found myself investing more time than intended in out-of-scope code exploration and extensive documentation reading.

1. Code Comments Comments were excellent in providing more information about the specific logic.

2. Documentation The documentation was well-written, although more information about how these tokens will be used would have been beneficial.

3. Organization The codebase is very mature and well-organized, with clear distinctions between the functions and their purposes.

4. Testing

• Unit Tests: The unit tests were sophisticated enough to uncover basic bugs. This is sufficient since the codebase was simple and didn't have any major complications or unnecessary complexity. I wrote some tests in Foundry, and the code behaved as expected. It can be noted that more tests are always better, as some tests were too simple, and some scenarios were not tested.

ERC20MultiDelegate

This is the main contract in the small system of votes. It is an ERC1155 token contract that implements ERC20Votes internally. It operates on the premise that when users want to delegate their votes to someone else, it creates an ERC20ProxyDelegator and sends those tokens in, while the ERC20ProxyDelegator delegates them to the delegate. After the tokens are sent and delegated, it mints the delegator ERC1155 tokens with the ID of the delegatee (as uint256(uint160(delegatee))). This way, tokens delegated to the same person can be transferred to other addresses while the delegate remains the same.

ERC20ProxyDelegator

This is the smaller of the two contracts. It is deployed when a user has received some vote tokens through ERC20MultiDelegate. They are generated one per user and store all of the vote tokens, delegating them to the user for whom they were deployed.

Architecture Recommendations

The contracts provided to us were sufficient to achieve the system's intended goals. They were quite simple, and the code was straightforward, making the audit a pleasant experience.

Positive Suggestions:

1. One potential enhancement I recommend is implementing a way for ERC1155 to work with approvals. This would allow users to approve the NFTs and authorize approved users to manage them, such as changing the delegated user, lowering or increasing votes, and so on.

Some things that may want a second look from the devs:

1. Include invariance and fuzz tests - These are essential for a comprehensive system.

Centralization risk

The code in its current version is decentralized to the maximum extent. The owner only has the ability to change the URI, which has no effect on the system and is used solely for visuals. The contracts are straightforward and provide users with maximum decentralization, making every action user-dependent. With the current control limit on the owner, even if they are compromised, nothing harmful can happen to the system, as they have no control over the mechanisms.

Systemic Risks

The current state of the protocol is very secure. I would recommend that the DAO and its proposal generation be examined in conjunction with this code for a full comprehensive audit. For now, the current part of the system seems secure.

Nonetheless, it is crucial to exercise caution when adding new features to the code, especially those involving economic incentives. While they can enhance the protocol's performance and attract users, they also tend to be the most vulnerable points, susceptible to exploitation if not designed and implemented with the utmost care.

Some potential risks may include:

1. Quadratic voting may be less efficient - This contract makes quadratic voting less efficient since, with less gas, we can achieve a higher spread on the tokens, thereby increasing their efficiency in a quadratic voting system. This makes it more likely that users will attempt Sybil attacks on the DAO and proposals.

Conclusion

In general, the ENS exhibits an interesting and well-developed architecture. I believe the team has done a good job regarding the code. It is also highly recommended that the team continues to invest in security measures such as mitigation reviews, audits, and bug bounty programs to maintain the security and reliability of the project.

Time spent:

7 hours