Commit hash: 4d2599cfe1c7d2647c26750cf547631a896dd518

Audited by: Parth Patel (Parth#7949)

# Issues

## FooToken

**[H-1]** `delegateWithSignature` doesn't check if `signatory` is blacklisted.

On line [206-211], FooToken.sol has the following code:

bytes32 structHash = keccak256(abi.encode(DELEGATION_TYPEHASH, newDelegate, nonce, expiry));
bytes32 digest = keccak256(abi.encodePacked('\x19\x01', domainSeparator, structHash));
address signatory = ecrecover(digest, v, r, s);
require(signatory != address(0), 'IT_INVALID_SIGNATURE');
require(nonce == nonces[signatory]++, 'IT_INVALID_NONCE');
_delegateFrom(signatory, newDelegate);

`delegateWithSignature` function generates `signatory` from the signature params `v`, `r` and `s` and try to delegate the voting power to `newDelegate` which is one of the function argument. However, this doesn't check if the `signatory` is blacklisted. Due to this, some blacklisted account can delegate their voting power to alternate addresses and vote.

Consider: Consider adding a check for the signatory to be blacklisted before delegating votes.

**[H-2]** `_mint` updates the voting balance of the address the tokens are minted to and not the delegate of that address.

On line [73-80], FooToken.sol has the following code:

function _mint(address to, uint256 _amount) internal {
uint96 amount = safe96(_amount);
totalSupply = totalSupply.add(_amount);
balances[to] = balances[to].add96(amount);
emit Transfer(address(0), to, _amount);

_updateVotes(address(0), to, amount);
}

`mint` function mints the tokens to address `to`. But it updates the vote of address `to` which is not desired. If the address `to` has delegated its voting power to some other account, then that account should get votes.

Consider: using `getDelegate(to)` to be argument of `_updateVotes` instead of `to`.

Fixed line:

    _updateVotes(address(0), getDelegate(to), amount);

**[H-3]** `mint` function not being override.

On line [67-71], FooToken.sol has the following code:

function mint(address to, uint256 _amount) external override {
require(isMinter[msg.sender], 'IT_ONLY_WHITELISTED');
require(!isBlacklisted[msg.sender] && !isBlacklisted[to], 'IT_BLACKLISTED');
_mint(to, _amount);
}

`mint` function using `override` keyword means that it expects that function to be implemented in the parent contract(i.e. `Votes`). But `Votes` contract does not implement `mint` function. Due to this syntax error, the code is resulting in a syntax error.

Consider: Remove `override` keyword from `mint` function.

**[M-4]** Use two-step ownership change for changing the owner of the contract.

On line [55-59], FooToken.sol has the following code:

function setOwner(address _owner) external {
require(msg.sender == owner, 'IT_FORBIDDEN');
owner = _owner;
emit OwnerSet(owner);
}

If `owner` by mistake grants the ownership to some unknown/undesired address, then the contract is prone to become uncontrolled. Instead use two-step ownership change where a new owner can claim the ownership and after that only, ownership changes. This is useful for this contract where a lot of things are dependent on the owner.

Consider: Consider using two-step change for changing the owner.

**[L-5]** `delegateWithSignature` checks if `msg.sender` is blacklisted.

On line [200], FooToken.sol has the following code:

require(!isBlacklisted[msg.sender] && !isBlacklisted[newDelegate], 'IT_BLACKLISTED');

`delegateWithSignature` function checks if `msg.sender` is blacklisted or not. However, this function will be called by `relayer` which should not be stopped to delegate the signature of some other account. The goal of `relayer` is just to relay some other account's signature by paying gas. This should not be stopped even if `relayer` is blacklisted.

Consider: Consider removing this check for `relayer`.

**[L-6]** The contract is designed in such a way that all tokens are assumed to fit in `uint96`, but uses `uint256` in all calculations.

Consider the following lines of FooToken.sol:

uint256 public totalSupply; //line 22
mapping(address => uint96) internal balances; //line 27
constructor(address account, uint256 _initialAmount) //line 40
function _mint(address to, uint256 _amount) internal { //line 73
function burn(uint256 _amount) external { //line 88
function approve(address spender, uint256 _amount) external returns (bool) { //line 99
function increaseAllowance(address spender, uint256 _extraAmount) external returns (bool) { //line 116
function decreaseAllowance(address spender, uint256 _subtractedAmount) external returns (bool) { //line 123
function transfer(address to, uint256 _amount) external returns (bool) { //line 132
function transferFrom(
address from,
address to,
uint256 _amount
) { //line 139-143

`line 27` assumes that `balances` of any account can be fit in `uint96` and doesn't exceed that. However, `totalSupply` is declared as `uint256`. Also, all the above lines shown in the snippet take arguments of `amount` or `votes` as `uint256` and in the functions, the function argument is cast to `uint96`. So, it's recommended to follow the standard type for all related variables.

Consider: Consider making the function arguments as `uint96` in the above snippet instead of typecasting it later.

## SafeMath

**[H-1]** `ceil_div` doesn't perform correct calculation.

On line [33-40], SafeMath.sol has the following code:

function ceil_div(uint256 a, uint256 b) internal pure returns (uint256 c) {
c = div(a, b);
if (c == mul(a, b)) {
return c;
} else {
return add(c, 1);
}
}

`ceil_div(6,3)` returns `3` but it should return `2`. That is because it should check whether `a == mul(b,c)` instead of checking `c == mul(a,b)`.

Consider: Using the above check in the function.