search

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

0 stars 0 forks source link

QA Report #190

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Missing zero address validation

If contract miss this zero check address validation there is chance that contract will loose some functionality

 For example:      File:   contracts/governance/Pausable.sol

 55   function _setPauseGuardian(address newPauseGuardian) internal {
 56   address oldPauseGuardian = pauseGuardian;
 57   pauseGuardian = newPauseGuardian;
 58   emit NewPauseGuardian(oldPauseGuardian, pauseGuardian);
 59         }

Below instances also lacks zero address check.

APPROVE should be replaced with SAFEAPPROVE or SAFEINCREASEALLOWANCE()/SAFEDECREASEALLOWANCE()

approve is subject to a known front-runnning attack. Consider using safeApprove instead.

File: contracts/gateway/BridgeEscrow.sol   #1
29        graphToken().approve(_spender, type(uint256).max);

Use of Block.timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Navigate to the following contracts.

File: contracts/governance/Pausable.sol   #1
32            lastPausePartialTime = block.timestamp;

public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents’ functions and change the visibility from externalto public

File: contracts/upgrades/GraphProxyAdmin.sol   #1
55    function getProxyAdmin(IGraphProxy _proxy) public view returns (address) {

Return values of transferFrom() not checked

Not all IERC20 implementations revert() when there’s a failure in transfer()/transferFrom(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually making a payment

File: contracts/gateway/L1GraphTokenGateway.sol   #1
235                token.transferFrom(from, escrow, _amount);

Event is missing indexed fields

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it’s not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.

File: contracts/governance/Pausable.sol   #1
19    event PartialPauseChanged(bool isPaused);

Floating Pragma

Use a more recent version of solidity

Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions