MohammedRizwan - `Lender.sol` is not fully compliant with `EIP2612`

[sherlock-admin2]

MohammedRizwan

medium

Lender.sol is not fully compliant with EIP2612

Summary

Lender.sol is not fully compliant with EIP2612

Vulnerability Detail

Per the contest readme.md, Lender.sol is compliant with EIP2612.

Is the code/contract expected to comply with any EIPs? Are there specific assumptions around adhering to those EIPs that Watsons should be aware of? The Lender complies with ERC4626 and EIP2612.

As per the ERC-2612 which is used as permit Extension for EIP-20 Signed Approvals.

Specification Compliant contracts must implement 3 new functions in addition to EIP-20: function permit(address owner, address spender, uint value, uint deadline, uint8 v, bytes32 r, bytes32 s) external function nonces(address owner) external view returns (uint) function DOMAIN_SEPARATOR() external view returns (bytes32)

1) Permit() is used in contract and the function is incompliance with EIP2612. 2) nonces() is missing therefore the Lender.sol is not in compliance with EIP2612. It should be noted here that nonce function is MUST requirement of EIP2612. Therefore this requirement can not be omitted. Per the defination of MUST as per RFC-2119

MUST This word, or the terms "REQUIRED" or "SHALL", mean that the definition is an absolute requirement of the specification.

Therefore, nonce() must be added in contract for proper compliance.

3) DOMAIN_SEPARATOR() is used incorrectly in contract. It should be noted that Lender.sol inherits Ledger.sol and DOMAIN_SEPARATOR() is a part of Ledger.sol

DOMAIN_SEPARATOR() used in the contract is given as below,

    function DOMAIN_SEPARATOR() public view returns (bytes32) {
        return
            keccak256(
                abi.encode(
                    keccak256("EIP712Domain(string version,uint256 chainId,address verifyingContract)"),
                    keccak256("1"),
                    block.chainid,
                    address(this)
                )
            );
    }

The above given function does not comply the EIP2612 as the EIP2612 states,

DOMAIN_SEPARATOR is defined according to EIP-712. The DOMAIN_SEPARATOR should be unique to the contract and chain to prevent replay attacks from other domains, and satisfy the requirements of EIP-712.

The DOMAIN_SEPARATOR should be like this,

DOMAIN_SEPARATOR = keccak256(
    abi.encode(
        keccak256('EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)'),
        keccak256(bytes(name)),
        keccak256(bytes(version)),
        chainid,
        address(this)
));

Therefore, DOMAIN_SEPARATOR() should be corrected.

Impact

Lender.sol breaks the design integration with EIP2612 and could result in expected behaviour which is not desired. Being EIP2612 is the core requirement of Lender.sol

Code Snippet

https://github.com/sherlock-audit/2023-10-aloe/blob/main/aloe-ii/core/src/Ledger.sol#L128

Tool used

Manual Review

Recommendation

Correctly follow EIP2612. Add the nonces() function which is a must requirement here also correct the DOMAIN_SEPARATOR() function as stated in EIP2612

[haydenshively]

Both nonces and DOMAIN_SEPARATOR are implemented in Ledger. Lender inherits from Ledger, so it is EIP2612 compliant.

[sherlock-admin2]

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

MohammedRizwan commented:

valid medium since issue is breaking core EIP requirement in lender.sol and valid per sherlock rule.

[0xRizwan]

nonce() function is missing in Ledger. sol, However there is nonces mapping is present in Ledger.sol. EIP2612 has MUST requirement of nonce() function, therefore the contract is not fully compliant with EIP-2612. Further, the DOMAIN_SEPARATOR() used is incorrect and does not follow EIP-2612 which says DOMAIN_SEPARATOR is defined according to EIP-712.

Per the contest readme.md, Lender.sol is compliant with EIP2612.

Is the code/contract expected to comply with any EIPs? Are there specific assumptions around adhering to those EIPs that Watsons should be aware of? The Lender complies with ERC4626 and EIP2612.

Per sherlock rules,

EIP Compliance: For issues related to EIP compliance, the protocol & codebase must show that there are important external integrations that would require strong compliance with the EIP's implemented in the code. The EIP must be in regular use or in the final state for EIP implementation issues to be considered valid

Therefore, i believe this issue is valid medium.

[jingyi2811]

Escalate

Escalate on behalf of 0xRizwan

[sherlock-admin2]

Escalate

Escalate on behalf of 0xRizwan

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[roguereddwarf]

@jingyi2811

According to EIP712, the domain separator can be chosen as needed:

With regards to the nonce, the nonces mapping is public so it's exposed just like a function.

Both of your arguments are invalid and the issue as a whole is invalid.

[0xRizwan]

@roguereddwarf

I agree with your point, nonce mapping will return the output similar to function but if you see, EIP-2612 it explicitely indicates to add nonce() function. The requirement here is "MUST" and not "SHOULD".

Further the mapping would be the part of nonce() function.

For example:

    mapping(address  => uint) private _nonces;

    function nonces(address owner) external view returns (uint) {
        return _nonces[owner];
    }

[Shogoki]

I agree with @roguereddwarf that this is invalid. The public variable creates a getter function that behaves the same. Any external integration would have no issue with that.

[Trumpero]

Planning to reject escalation and keep this issue invalid.

The contract already has a public mapping nonces that can be called externally like an external function.

[Evert0x]

Agree with Trumpero

mapping(address => uint256) public nonces;

Will automatically provide the function nonces(address) external view returns(uint256) function, so the issue is invalid.

Will reject escalation and keep issue state as is.

[Czar102]

Result: Invalid Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• jingyi2811: rejected