search

code-423n4 / 2022-10-thegraph-findings

0 stars 0 forks source link

[NAZ-M1] `GraphTokenUpgradeable.permit()` Should Always Check `recoveredAddress != 0` #243

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-10-thegraph/blob/main/contracts/l2/token/GraphTokenUpgradeable.sol#L94

Vulnerability details

Impact

The GraphTokenUpgradeable.permit() function ignores the recoveredAddress != 0 check if isApprovedForAll[owner][recoveredAddress] is true. If a user accidentally set the zero address as the operator, tokens can be stolen by anyone as a wrong signature yield recoveredAddress == 0.

Tools Used

Manual Review

Recommended Mitigation Steps

Change the require logic to recoveredAddress != address(0) && (recoveredAddress == owner) || isApprovedForAll[owner][recoveredAddress]).

0xean commented 1 year ago

closing as invalid, this is handled in the OZ ECDSA.recover() function

0xean commented 1 year ago

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/8d908fe2c20503b05f888dd9f702e3fa6fa65840/contracts/utils/cryptography/ECDSA.sol#L154