EIP-5805 non-compliance

Summary

EpochBasedVoteToken and EpochBasedInflationaryToken fail to comply with the inherited ERC5805 standard.

Vulnerability Detail

EpochBasedVoteToken and EpochBasedInflationaryToken inherit ERC5805 but are non-compliant in two ways:

The EIP states: "Tokens that are delegated to address(0) should not be tracked." However, this is not the case. Instead, tokens that are delegated to address(0) are self-delegated:

// `delegatee_` will be `delegator_` (the default) if `delegatee_` was passed in as `address(0)`.
address newNonZeroDelegatee_ = _getDefaultIfZero(newDelegatee_, delegator_);

function _getDefaultIfZero(address input_, address default_) internal pure returns (address) {
    return input_ == address(0) ? default_ : input_;
}

The EIP also states: "For all accounts a != 0 and all timestamp t < clock, getPastVotes(a, t) SHOULD be the sum of the “balances” of all the accounts that delegated to a when clock overtook t." This is not the case with EpochBasedInflationaryVoteToken since unrealized inflation must first be manually realized by calling _sync to update past balances:

function _sync(address account_) internal virtual {
    // Realized the account's unrealized inflation since its last sync.
    _addBalance(account_, _getUnrealizedInflation(account_, _clock()));
}

Impact

Failure to comply to EIP's can lead to unexpected effects.

Code Snippet

Tool used

Manual Review

Recommendation

Comply to the EIP specifications or otherwise clearly indicate that the contract is not fully ERC5805 compliant.

sherlock-admin3 commented 7 months ago

1 comment(s) were left on this issue during the judging contest.

takarez commented:

as per the Readme this should be a valid; medium(5)

deluca-mike commented 7 months ago

1 is an invalid issue.

First, the EIP says that assets can be delegated to address(0), not that this must be possible. The assets cannot be delegated to address(0) here.

Second, it says assets that are delegated to address(0) should not be tracked. Aside from the should keyword, you'll see that assets are never delegated to address(0). They are always delegated to some address, even by default.

deluca-mike commented 7 months ago

2 is also an invalid issue.

"The key words “MUST”, “MUST NOT”, “REQUIRED”, “SHALL”, “SHALL NOT”, “SHOULD”, “SHOULD NOT”, “RECOMMENDED”, “MAY”, and “OPTIONAL” in this document are to be interpreted as described in RFC 2119."

"SHOULD This word, or the adjective "RECOMMENDED", mean that there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course."

There are limitations that prevent the implementation from meeting this recommendation.

sherlock-audit / 2023-10-mzero-judging

0xkaden - EIP-5805 non-compliance #19