return value of 0 from ecrecover is not checked

[hats-bug-reporter[bot]]

Github username: -- Submission hash (on-chain): 0xe3c62c6546edd8b652a731dbcbaff6f24d971a29fe18cdbb29ce88ff9487b030 Severity: high severity

Description:

Impact

Solidity’s ecrecover returns 0 if signature is invalid.

The AToken.sol contract permit() fuction has used ecrecover which does not perform zero address check on ecrecover’s return value and returns as-it-is.

function permit(
        address owner,
        address spender,
        uint256 value,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) external {
        require(owner != address(0), "INVALID_OWNER");
        //solium-disable-next-line
        require(block.timestamp <= deadline, "INVALID_EXPIRATION");
        uint256 currentValidNonce = _nonces[owner];
        bytes32 digest = keccak256(
            abi.encodePacked(
                "\x19\x01",
                DOMAIN_SEPARATOR(),
                keccak256(
                    abi.encode(
                        PERMIT_TYPEHASH,
                        owner,
                        spender,
                        value,
                        currentValidNonce,
                        deadline
                    )
                )
            )
        );
        require(owner == ecrecover(digest, v, r, s), "INVALID_SIGNATURE");
        //@audit-issue 0 address check
        _nonces[owner] = currentValidNonce.add(1);
        _approve(owner, spender, value);
    }

As seen above at L-464, ecrecover is used, however the signer result of 0 is not checked. If the signer is not checked to address(0) then it will assume a valid signature

As per Solidity documentation:

"recover address associated with the public key from elliptic curve signature, return zero on error"

https://docs.soliditylang.org/en/v0.8.20/cheatsheet.html#mathematical-and-cryptographic-functions

Now you can supply invalid input parameters to AToken.transmit() function, which will give result as 0.

This zero-address check is present in Openzeppelin ECDSA.sol: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/1b27c13096d6e4389d62e7b0766a1db53fbb3f1b/contracts/utils/cryptography/ECDSA.sol#L170-L173

Proof of Concept

Reference to similar High severity finding in Swivel audit by Code4rena: https://github.com/code-423n4/2021-09-swivel-findings/issues/61

Recommended Mitigation Steps

1)Recommend to use openzeppelin ECDSA.sol OR

2)Add zero-address check for ecrecover return value

[Nabeel-javaid]

Tried my best to explain in detail

[ksyao2002]

Thanks for the detailed report. You mentioned that the signer (which I assume is owner) is not checked to address(0), but it is checked on the first line of the permit function. Regardless, I don't think there can be a loss of funds since you cannot sign as address(0).

Please let me know if I am missing anything. We didn't change it from Aave's original implementation, which was audited many times and has never been exploited from this, so I don't think this is a vulnerability.

[Nabeel-javaid]

Thanks for the detailed report. You mentioned that the signer (which I assume is owner) is not checked to address(0), but it is checked on the first line of the permit function. Regardless, I don't think there can be a loss of funds since you cannot sign as address(0).

Please let me know if I am missing anything. We didn't change it from Aave's original implementation, which was audited many times and has never been exploited from this, so I don't think this is a vulnerability.

No, i'm talking about the return value of ecrecover, it can give the result as 0 as explained above also if you look at this link then you can see that indeed this vulnerability is high severity and atleast med considering the situation. Also I'ven't fully read about aave so I can't say anything about that. The best possible mitigation of all ecrecover related issues is to use ECDSA.sol

[ksyao2002]

Let's assume ecrecover does return 0. The only way it can affect the protocol negatively is if owner is also address(0), then the require(owner == ecrecover...) would pass erroneously. However, checking owner for address(0) is done on the first line of the permit function, so there's no way that the ecrecover can cause vulnerability in the way that you state.