flawed validation checks for permitForAll() function

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-aave-lens/blob/aaf6c116345f3647e11a35010f28e3b90e7b4862/contracts/core/base/LensNFTBase.sol#L159-L168 https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/base/LensNFTBase.sol#L111 https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/base/LensNFTBase.sol#L78

Vulnerability details

Impact

In LensNFTBase.sol the permit() and permitForAll() functions both use the _validateRecoveredAddress() to recover the address of the signer. In _validateRecoveredAddress() it uses an OR operator "||" in its 2 security checks when in fact both checks should have their own require checks independent of one another. Using the "||" operator here is dangerous because this means that one of the checks can pass while the other fails leaving space for the function to be exploited and since this functionality deals with permitting spenders of tokens it can be costly.

Proof of Concept

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/base/LensNFTBase.sol#L159

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/base/LensNFTBase.sol#L111

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/base/LensNFTBase.sol#L78

Tools Used

Manual code review

Recommended Mitigation Steps

Add the following checks separately instead of using || in the _validateRecoveredAddress() function

Use this: require(recoveredAddress != address(0), "Cannot be zero address); require(recoveredAddress == expectedAddress, "Addresses don't match");

[Zer0dot]

This is just wrong, the code uses modern errors not requires.

[0xleastwood]

The use of || WILL check all the required conditons. There is no need to separate them.