LPFarming, yVault and yVaultLPFarming contracts' white list checks are performed with isContract() and can be surpassed. isContract() can only be used for positive confirmations, i.e. filtering out EOAs.
Setting high severity as it is a direct access control logic loophole for the number of user facing functions.
Proof of Concept
OZ documents 'it is unsafe to assume that an address for which this function returns false is an externally-owned account (EOA) and not a contract':
I.e. a contract can choose not to identify itself as a contract, but EOA can't choose to be identified as a contract, so only the cases where EOA should be filtered out are valid for isContract().
However, LPFarming, yVault and yVaultLPFarming control that an address isn't a contract with Address.isContract():
Consider removing the filtration of contract callers altogether, focusing on direct attack surfaces (nonReentrant is a great example).
If there are strong enough considerations for filtering out of the contracts consider using tx.origin == msg.sender, which might not be deprecated for a long enough time as there is a reasonably big number of use cases around and backward compatibility is a strong argument in the ongoing discussion.
spaghettieth
commented
2 years ago
Lines of code
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L87 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/yVaultLPFarming.sol#L56 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L63
Vulnerability details
Impact
LPFarming, yVault and yVaultLPFarming contracts' white list checks are performed with isContract() and can be surpassed. isContract() can only be used for positive confirmations, i.e. filtering out EOAs.
Setting high severity as it is a direct access control logic loophole for the number of user facing functions.
Proof of Concept
OZ documents 'it is unsafe to assume that an address for which this function returns false is an externally-owned account (EOA) and not a contract':
https://docs.openzeppelin.com/contracts/3.x/api/utils#Address-isContract-address-
https://forum.openzeppelin.com/t/how-to-write-an-onlynoncontract-modifier-function-cant-be-called-by-a-contract/3042/4
This way the approach implemented in OZ's Address can be used for positive confirmations only:
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L36-L42
I.e. a contract can choose not to identify itself as a contract, but EOA can't choose to be identified as a contract, so only the cases where EOA should be filtered out are valid for isContract().
However, LPFarming, yVault and yVaultLPFarming control that an address isn't a contract with Address.isContract():
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L87
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/yVaultLPFarming.sol#L56
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L63
Recommended Mitigation Steps
Consider removing the filtration of contract callers altogether, focusing on direct attack surfaces (
nonReentrantis a great example).If there are strong enough considerations for filtering out of the contracts consider using
tx.origin == msg.sender, which might not be deprecated for a long enough time as there is a reasonably big number of use cases around and backward compatibility is a strong argument in the ongoing discussion.