sherlock-audit / 2023-10-aloe-judging

9 stars 6 forks source link

OxZ00mer - The Lender contract is not fully EIP-4626 compliant, leading to confusion when interacting with it #136

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

OxZ00mer

medium

The Lender contract is not fully EIP-4626 compliant, leading to confusion when interacting with it

Summary

The Lender.sol contract, which inherits from Ledger.sol, does not return true on supportsInterface(ERC20). This issue makes it incompatible with the EIP-4626 specification, potentially causing confusion when other protocols integrate with a Lender.sol vault.

Vulnerability Detail

The Lender.sol vault contract should be fully EIP-4626 compliant, meeting all aspects of the specification except for one requirement:

All EIP-4626 tokenized Vaults MUST implement EIP-20 to represent shares.

For the contract to be fully EIP-20 compliant, it should also support ERC20 as per EIP-165, which it currently does not.

function supportsInterface(bytes4 interfaceId) external pure returns (bool) {
    // @audit it will return false on IERC20
    return
        interfaceId == type(IERC165).interfaceId ||
        interfaceId == type(IERC2612).interfaceId ||
        interfaceId == type(IERC4626).interfaceId;
}

More information about EIP-165 can be found here:

[EIP-165](https://eips.ethereum.org/EIPS/eip-165)

Impact

The vault is expected to be fully EIP-4626 compliant, and other protocols integrating with it assume this as well. However, the current state of affairs can lead to confusion and issues during such integrations, potentially resulting in a loss of value.

Code Snippet

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

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

Tool used

Manual Review

Recommendation

It is recommended to modify the supportsInterface() function in Ledger.sol to ensure full EIP-4626 compliance:

function supportsInterface(bytes4 interfaceId) external pure returns (bool) {
    return
        interfaceId == type(IERC20).interfaceId ||
        interfaceId == type(IERC165).interfaceId ||
        interfaceId == type(IERC2612).interfaceId ||
        interfaceId == type(IERC4626).interfaceId;
}

These changes will address the issue and align the vault with the EIP-4626 specification.

Duplicate of #149

sherlock-admin2 commented 1 year ago

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

MohammedRizwan commented:

invalid as IERC4626 already inherits IERC20 Therefore the contract implements both interfaces.

cvetanovv commented 1 year ago

Escalate

I am escalating on behalf of Watson. This is not a duplicate of #149 and should be considered separately. Should be a valid medium.

sherlock-admin2 commented 1 year ago

Escalate

I am escalating on behalf of Watson. This is not a duplicate of #149 and should be considered separately. Should be a valid medium.

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.

Shogoki commented 1 year ago

Escalate

I am escalating on behalf of Watson. This is not a duplicate of #149 and should be considered separately. Should be a valid medium.

I do not think this is a valid Medium. From the Sherlock Docs:

EIP compliance with no integrations: If the protocol does not have external integrations then issues related to code not fully complying with the EIP it is implementing and there are no adverse effects of this, are considered informational

As far as i know there are no external integrations for the Vault contract, yet.

Banditx0x commented 12 months ago

Does not break any core functionality or cause loss of funds. Not a valid medium

Trumpero commented 12 months ago

Planning to reject escalation of @cvetanovv and keep issue invalid.

ERC4626 already inherits ERC20, so interfaceId of IERC4626 already covers IERC20.

Czar102 commented 11 months ago

Also, I don't think any of the eips enforced by the readme require the contract to be eip-165 compliant.

Czar102 commented 11 months ago

Result: Invalid Duplicate of #149

sherlock-admin2 commented 11 months ago

Escalations have been resolved successfully!

Escalation status: