# Quillhash Audit_mocks

**Repo:** https://github.com/Quillhash/Audit_mocks

https://github.com/Quillhash/Audit_mocks/blob/main/MGGovToken.sol

## General

## `MGGovToken.sol`

### [Critical] Centralization Risk can be created by owner

**File(s)**: [`MGGovToken.sol`](https://github.com/Quillhash/Audit_mocks/blob/main/MGGovToken.sol)

**Description**: The function `mint` can only called by the owner of the contract. A malicious owner will basically call the `delegate` function and there the owner will pass the own address and mint all the tokens, basically all the votes are under the owner only and which makes it completely centralized and leads to a malicious protocol. The `burn` function can only be called by the owner and malicious owner tries to burn all the tokens of all other delegators which leads to decrease in the number of votes of the respective delegatee.

**Recommendation(s)**: Consider making `mint` and `burn` functions accessible to everyone and also put a limit for minting for each address and also put price per mint and also `mint` and `burn` function should be under the control of each delegator passing it for each delegatee

---
### [Critical] Transfer and TransferFrom function will not update the number of votes of the delegatee

**File(s)**: [`MGGovToken.sol`](https://github.com/Quillhash/Audit_mocks/blob/main/MGGovToken.sol)

**Description**: The contract `MockGovToken` inherits BEP20 Standard and here we can use a `transfer` and `transferfrom` functions, so for example if Alice address has balance of 1000 tokens and Alice as a delegator passes all the 1000 tokens to the delegatee and therfore the no of votes of delegatee of Alice would be 1000. Then Alice transfers all the 1000 tokens to Bob and now the balance of Alice is 0 but the number of votes of delagatee of Alice still remains 1000 only since the `_moveDelegates` function is not called.

**Recommendation(s)**: Create a new function `_transfer` and in that function call the `_moveDelegates` function

   function _transfer(address _to, uint256 _amount) external 
   {
        transfer(_to,_amount);
        _moveDelegates(_delegates[msg.sender], address(0), _amount);
   }

---
### [High] No updation of `nonces` while calling `delegate` function will lead to signature attack while calling `delegateBySig` function

**File(s)**: [`MGGovToken.sol`](https://github.com/Quillhash/Audit_mocks/blob/main/MGGovToken.sol)

**Description**: Lets take an example suppose Alice has signed a first transaction as a delegator for Bob to be as a delegatee with nonce = 0. Before Bob calling the `delegateBySig` function suppose Alice called the `delegate` function and passes the address a which is not Bob's address then the new delegatee of Alice would be not Bob address. Now when Bob calls the `delegateBySig` function then if the signature expiry is not passed made by Alice then the `delegateBySig` function passing nonce = 0 will be executed successfully leading to the change of current delegatee of Alice to Bob's address.

**Recommendation(s)**: Since both the `delegateBySig` and
`delegate` functions calls the `_delegate` function, we can write the code as `++nonces[delegator]` at the end of the function and in the `delegateBySig` function already there exists `nonce == nonces[signatory]++` can be replaced with `nonce == nonces[signatory]`.

---
### [Medium] Effects the protocol by logging the event

**File(s)**: [`MGGovToken.sol`](https://github.com/Quillhash/Audit_mocks/blob/main/MGGovToken.sol)

**Description**: In the `_delegate` function emitting the `DelegateChanged` event before calling the `_moveDelegates` function will log the event even if the `_moveDelegates` function fails to execute which effects the protocol event logging.

**Recommendation(s)**: Change the exisiting code to this code

   
      function _delegate(address delegator, address delegatee) private {
        address currentDelegate = _delegates[delegator];
        uint256 delegatorBalance = balanceOf(delegator); 
        require(delegatorBalance != 0 ,"Zero Balance");
        _delegates[delegator] = delegatee;

        _moveDelegates(currentDelegate, delegatee, delegatorBalance);
        ++nonces[delegator];
         emit DelegateChanged(delegator, currentDelegate, delegatee); // emit afterwards or else even if movedelegates fails it emits as delegate change
    }

---

---
### [Info] Deadline check should be done first

**File(s)**: [`MGGovToken.sol`](https://github.com/Quillhash/Audit_mocks/blob/main/MGGovToken.sol)

**Description**: In the function `delegateBySig` `now <= expiry` is checked at the last line and other calculations were done first. When the signature is already finished then it is unnesscary to calculate the other things before checking the signature expiry which leads to a lot gas wastage.

**Recommendation(s)**: Replace the exisiting code to the below code given

   
  function delegateBySig(
        address delegatee,
        uint256 nonce,
        uint256 expiry,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) external {
      
        require(now <= expiry, "MGToken::delegateBySig: signature expired"); // This should be checked first only to save gas and if it expired already then unneccesary calculation can avoided.
        bytes32 domainSeparator = keccak256(
            abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name())), getChainId(), address(this))
        );

        bytes32 structHash = keccak256(abi.encode(DELEGATION_TYPEHASH, delegatee, nonce, expiry));

        bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));

        address signatory = ecrecover(digest, v, r, s);
        require(signatory != address(0), "MGToken::delegateBySig: invalid signature"); // address check using assembly and also you revert/error instead of require
        require(nonce == nonces[signatory], "MGToken::delegateBySig: invalid nonce"); // 
        return _delegate(signatory, delegatee);
    }

---
### [Info] Anyone can call the `delegateBySig` function

**File(s)**: [`MGGovToken.sol`](https://github.com/Quillhash/Audit_mocks/blob/main/MGGovToken.sol)

**Description**: The `delegateBySig` function can be called by anyone and instead of delegatee address, other address can successfully execute the function if they know all the inputs to be passed at any time.

**Recommendation(s)**: Replace the exisiting code to the below code given

   
  function delegateBySig(
        address delegatee,
        uint256 nonce,
        uint256 expiry,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) external {
        require(msg.sender == delegatee,"Not a delegatee");// Non critical/ Low Finding since ignoring this will not affect the protocol.
        require(now <= expiry, "MGToken::delegateBySig: signature expired"); // This should be checked first only to save gas and if it expired already then unneccesary calculation can avoided.
        bytes32 domainSeparator = keccak256(
            abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(name())), getChainId(), address(this))
        );

        bytes32 structHash = keccak256(abi.encode(DELEGATION_TYPEHASH, delegatee, nonce, expiry));

        bytes32 digest = keccak256(abi.encodePacked("\x19\x01", domainSeparator, structHash));

        address signatory = ecrecover(digest, v, r, s);
        require(signatory != address(0), "MGToken::delegateBySig: invalid signature"); // address check using assembly and also you revert/error instead of require
        require(nonce == nonces[signatory], "MGToken::delegateBySig: invalid nonce"); // 
        return _delegate(signatory, delegatee);
    }

---

### [Info] Add a function to get the block number

**File(s)**: [`MGGovToken.sol`](https://github.com/Quillhash/Audit_mocks/blob/main/MGGovToken.sol)

**Description**: Users calling `getPriorVotes` function need to pass the blocknumber and guessing the block number is really hard and user will not know what is the limit of block number and it takes a lot of tries to guess the right one and get the `PriorVotes`.

**Recommendation(s)**: Add the below function to get the block number easily and it will be helpful for the users calling `getPriorVotes` function and try to search within the current blocknumber.

       function getblocknumber() external view returns(uint256) // using this function for get prior votes function will be easy
    {
        return block.number - 1;
    }

---
### [Info] Check can be done before

**File(s)**: [`MGGovToken.sol`](https://github.com/Quillhash/Audit_mocks/blob/main/MGGovToken.sol)

**Description**: In the line 161 the code check is done ie `srcRep != dstRep && amount > 0` in the `_moveDelegates` function

      
      function _moveDelegates(
        address srcRep,
        address dstRep,
        uint256 amount
    ) internal {
        if (srcRep != dstRep && amount > 0) {
            if (srcRep != address(0)) {
                // decrease old representative
                uint32 srcRepNum = numCheckpoints[srcRep];
                uint256 srcRepOld = srcRepNum > 0 ? checkpoints[srcRep][srcRepNum - 1].votes : 0;
                uint256 srcRepNew = srcRepOld.sub(amount);
                _writeCheckpoint(srcRep, srcRepNum, srcRepOld, srcRepNew);
            }

            if (dstRep != address(0)) {
                // increase new representative
                uint32 dstRepNum = numCheckpoints[dstRep];
                uint256 dstRepOld = dstRepNum > 0 ? checkpoints[dstRep][dstRepNum - 1].votes : 0;
                uint256 dstRepNew = dstRepOld.add(amount);
                _writeCheckpoint(dstRep, dstRepNum, dstRepOld, dstRepNew);
            }
        }
    }

**Recommendation(s)**: Replace `delegatee != _delegates[delegatee]` with `srcRep != dstRep` and also `delegatorBalance != 0` with `amount > 0` to save gas.

      
 function delegate(address delegatee) external {
        require(delegatee != _delegates[delegatee],"Already a delegatee"); // replaced code instead of srcRep != dstRep
        return _delegate(msg.sender, delegatee);
    }


function _delegate(address delegator, address delegatee) private {
        address currentDelegate = _delegates[delegator];
        uint256 delegatorBalance = balanceOf(delegator); 
        require(delegatorBalance != 0 ,"Zero Balance"); // replaced code instead of amount > 0
        _delegates[delegator] = delegatee;
        _moveDelegates(currentDelegate, delegatee, delegatorBalance);
        ++nonces[delegator];
         emit DelegateChanged(delegator, currentDelegate, delegatee); 

---

### [Best Practices] Avoid using strings with length greater than 32 bytes

**File(s)**: [`MGGovToken.sol`](https://github.com/Quillhash/Audit_mocks/blob/main/MGGovToken.sol)

**Description**: The `require(...)` string in the Line 94,95,96,191 are having more than 32 bytes.

**Recommendation(s)**: Consider reducing the length of this string up to 32 bytes or else revert and error can be used instead to save gas.

---
### [Best Practices] Use != 0 instead of > 0 for unsigned integer comparison

**File(s)**: [`MGGovToken.sol`](https://github.com/Quillhash/Audit_mocks/blob/main/MGGovToken.sol)

**Description**: In the Lines 107,166,170,178,193 the variable is compared as >0,

**Recommendation(s)**: Consider changing >0 to !=0 to save gas.

---

### [Best Practices] `Internal` functions to be changed to `Private` functions

**File(s)**: [`MGGovToken.sol`](https://github.com/Quillhash/Audit_mocks/blob/main/MGGovToken.sol)

**Description**: Functions like `getChainId`,`safe32`,`_writeCheckpoint`,`_moveDelegates`,`_delegate` are only called with contract but it is uses `Internal` keyword.

**Recommendation(s)**: Change the `Internal` functions to `Private` functions to save gas.

---

### [Best Practices] `Public` functions to be changed to `External` functions

**File(s)**: [`MGGovToken.sol`](https://github.com/Quillhash/Audit_mocks/blob/main/MGGovToken.sol)

**Description**: Functions like `mint`,`burn`, are only called outside the contract but it is uses `Public` keyword.

**Recommendation(s)**: Change the `Public` functions to `External` functions to save gas.

---

### [Best Practices] Default Values Not required to initialized

**File(s)**: [`MGGovToken.sol`](https://github.com/Quillhash/Audit_mocks/blob/main/MGGovToken.sol)

**Description**: In the Line 135 `uint32 lower=0` is unnecessary to assigned with the default value

**Recommendation(s)**: Change `uint32 lower=0` to `uint32 lower` to save gas.

---