# Security Audit of `LamboToken` contract.

## Conclusion

This audit was made by
Auditor: Vladimir Smelov <[e-mail]>.
Date: 2022-09-16

The client was acknowledged about all notes below.

## Scope

LamboToken.sol - https://testnet.bscscan.com/address/0x477975206744De34bE580AA5Efb4576011E8d232#code#F1#L1

## Methodology

1. Blind audit. Understand the structure of the code without reading any docs.
2. Ask questions to developers.
3. Run static analyzers.
4. Find problems with:
- backdoors;
- bugs;
- math;
- potential leaking of funds;
- potential locking of the contract;
- validate arguments and events;
- others.

## Result

### Static Analyzers reports

#### Mythx

https://drive.google.com/file/d/1gNHZJYe2W9l1_IzXIHmboVodDVMljOvU/view?usp=sharing

#### Slither

https://drive.google.com/file/d/1JVvjyPmRchTrU0KuJLE0AqREDMzaf_xB/view?usp=sharing

### Critical

#### CRITICAL-1. Infinite voting power may be generated.

Currently, there is no transfer of voting power on transfer token.

This lets anyone to accumulate infinite voting power by calling `transfer` and `delegate` multiple times.

Example:

def test_delegate(admin, users, lambo):
    amount = 1e6 * 1e18
    alice = users[0]
    bob = users[1]
    carol = users[2]
    mallory = users[3]
    lambo.transfer(alice, amount, {"from": admin})
    lambo.delegate(mallory, {"from": alice})
    lambo.transfer(bob, amount, {"from": alice})
    lambo.delegate(mallory, {"from": bob})
    lambo.transfer(carol, amount, {"from": bob})
    lambo.delegate(mallory, {"from": carol})
    assert lambo.getCurrentVotes(mallory) == 3 * amount # OMG!

##### Recommendation.
Consider transfering of voting power on every token transfer, like:

    function _afterTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal virtual override {
        super._afterTokenTransfer(from, to, amount);
        _moveVotingPower(_delegates[from], _delegates[from], amount);
    }

##### Status.
NEW

___

### Major

There are no major in the final version of the contract.

___

### Warning

#### WARNING-1. `delegateBySig` allows mallable signatures.

At:
- contracts/LamboToken.sol:97

The `ecrecover` EVM opcode allows for malleable (non-unique) signatures.

See also:

- https://0xsomeone.medium.com/b002-solidity-ec-signature-pitfalls-b24a0f91aef4
- https://github.com/ethereum/yellowpaper/pull/860
- https://docs.rs/secp256k1-abc-sys/0.1.2/secp256k1_abc_sys/fn.secp256k1_ecdsa_signature_normalize.html
- https://0xsomeone.medium.com/b002-solidity-ec-signature-pitfalls-b24a0f91aef4
- https://twitter.com/alexberegszaszi/status/1534461421454606336?s=20&t=H0Dv3ZT2bicx00hLWJk7Fg
- https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1622

In short, knowing the digest and the signature, mallory may generate another valid signature. In your particular contract it looks like it does not lead to any security problem, but accepting mallabale signatures is a bad practice and is essentially an implicit characteristic of the code.

BitCoin, Ethereum and OpenZeppelin and various libraries do not accept such signatures.

##### Recommendation.
Do not accept mallable signatures.

Use this - https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol#L132

function tryRecover(
    bytes32 hash,
    uint8 v,
    bytes32 r,
    bytes32 s
) internal pure returns (address, RecoverError) {
    // EIP-2 still allows signature malleability for ecrecover(). Remove this possibility and make the signature
    // unique. Appendix F in the Ethereum Yellow paper (https://ethereum.github.io/yellowpaper/paper.pdf), defines
    // the valid range for s in (301): 0 < s < secp256k1n ÷ 2 + 1, and for v in (302): v ∈ {27, 28}. Most
    // signatures from current libraries generate a unique signature with an s-value in the lower half order.
    //
    // If your library generates malleable signatures, such as s-values in the upper range, calculate a new s-value
    // with 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141 - s1 and flip v from 27 to 28 or
    // vice versa. If your library also generates signatures with 0/1 for v instead 27/28, add 27 to v to accept
    // these malleable signatures as well.
    if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) {
        return (address(0), RecoverError.InvalidSignatureS);
    }

    // If the signature is valid (and not malleable), return the signer address
    address signer = ecrecover(hash, v, r, s);
    if (signer == address(0)) {
        return (address(0), RecoverError.InvalidSignature);
    }

    return (signer, RecoverError.NoError);
}

##### Status.
NEW

___

#### WARNING-2. Lambo is a registred trademark.

You use Lambo as a name of your token.
However "Lambo" is a registred trademark

- https://www.trademarkelite.com/europe/trademark/trademark-detail/009047151/Lambo
- https://trademark.trademarkia.com/lambo-77238929.html

It may cause potential litigation in the Future.

##### Recommendation.
Consult with lawyers if it's OK to use registred trademark, to avoid legal risks.
##### Status.
NEW

___

#### WARNING-3. Avoid reinventing the wheel.

98% of LamboToken code could be removed just by using industry-standard, well-tested OpenZeppelin contracts
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/ERC20Votes.sol

If you don't want to use ERC20Votes you can at least consider usage of

- https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol
- https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/governance/utils/IVotes.sol
- https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Checkpoints.sol
- https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/EIP712.sol

you have reinvented a lot!

##### Recommendation.
Reduce the code base, use buggy-free OpenZeppelin.
##### Status.
NEW

___

#### WARNING-4. The usage of the return value of the function which returns nothing.

At:
- contracts/LamboToken.sol:49
- contracts/LamboToken.sol:101

you have

        return _delegate(signatory, delegatee);

but `_delegate` does not return anything. It's confusing.

##### Recommendation.
Remove the `return` statement.
##### Status.
NEW

___

#### WARNING-5. Contract will be locked after `2**32-1` block.

At:
- contracts/LamboToken.sol:203

after `2**32-1` block it will fail.

this is not a completely fictional situation. Modern blockchain may synchronize block very fast, many blockchains go beyond 1bps. So in case if you deploy your contract on blockchain with 1 block per 10ms, the contract will be blocked in `2**32 / 365 / 24 / 3600 / 100 ~ 1year` because block.number will hit `2**32`.

##### Recommendation.
Be aware of it. Do estimation for a specific blockchain how many years you have until the contract will be locked. For BSC it's about `2**32 / 365 / 24 / 3600 * 3 ~ 400 years`.
Or use uint256.
##### Status.
NEW

---

### Comment.

___

#### COMMENT-1. Floating solidity pragma.
At

- contracts/LamboToken.sol:3

pragma solidity ^0.8.0;

It is always recommended that pragma should be fixed to the version that you are intending to deploy your contracts with to be sure that the compile bytecode will work in the same way.

##### Recommendation.
Fix the pragma, e.g.:

pragma solidity 0.8.6;

##### Status.
NEW

___

#### COMMENT-2. Use tight structures.
At

- contracts/LamboToken.sol:11

    struct Checkpoint {
        uint32 fromBlock;
        uint256 votes;
    }

This structure has 256+32 bits, so it will use 2 storage slots.
However it's not likely that you will use totalSupply higher than `2**224-1` , so you can pack the struct into one storage slot with defining `uint224 votes;` and using a safeCast of votes.

##### Recommendation.
Replace with

    struct Checkpoint {
        uint32 fromBlock;
        uint224 votes;
    }

and

votes: SafeCast.toUint224(newVotes)

##### Status.
NEW

___

#### COMMENT-3. The usage of structs is costing you gas.
At

- contracts/LamboToken.sol:11

    struct Checkpoint {
        uint32 fromBlock;
        uint256 votes;
    }

even after `COMMENT-2` fix, the usage of the struct will cost you additional gas.

Read this - https://medium.com/@novablitz/storing-structs-is-costing-you-gas-774da988895e

You can save some gas by the usage of bits manipulations, check the mock contract here - https://goerli.etherscan.io/address/0x68c31efbf52144e5e02c1984ec31a9645d0eeea0#code

/**
 *Submitted for verification at Etherscan.io on 2022-09-14
*/

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.0;

contract CheckpointTest {
    struct Checkpoint {
        uint32 fromBlock;
        uint224 votes;
    }

    mapping (address => mapping (uint32 => Checkpoint)) public checkpointStructs;


    //     | <-- fromBlock (32 bits) --> | <-- votes (224 bits) --> |
    mapping (address => mapping (uint32 => uint256)) public checkpointBytes;
    uint256 constant internal BLOCK_MASK = type(uint256).max << 224;  // 111100000000
    uint256 constant internal VOTES_MASK = type(uint256).max >> 32;   // 000011111111

    function _writeCheckpointStruct(
        address delegatee,
        uint32 nCheckpoints,
        uint256 oldVotes,
        uint256 newVotes
    )
        external  // warning for test purposes only
    {
        require(newVotes < 2**224, "Lambo::_writeCheckpoint: newVotes exceeds 224 bits");
        require(block.number < 2**32, "Lambo::_writeCheckpoint: block number exceeds 32 bits");
        uint32 blockNumber = uint32(block.number);

        if (nCheckpoints > 0 && checkpointStructs[delegatee][nCheckpoints - 1].fromBlock == blockNumber) {
            checkpointStructs[delegatee][nCheckpoints - 1].votes = uint224(newVotes);  // safe conversion since it's already checked
        } else {
            checkpointStructs[delegatee][nCheckpoints] = Checkpoint(blockNumber, uint224(newVotes));  // safe conversion since it's already checked
        }
    }

    function _writeCheckpointBytes(
        address delegatee,
        uint32 nCheckpoints,
        uint256 oldVotes,
        uint256 newVotes
    )
        external  // warning for test purposes only
    {
        require(newVotes < 2**224, "Lambo::_writeCheckpoint: newVotes exceeds 224 bits");
        require(block.number < 2**32, "Lambo::_writeCheckpoint: block number exceeds 32 bits");

        if (nCheckpoints > 0 && (checkpointBytes[delegatee][nCheckpoints - 1] >> 224) == block.number) {
            uint256 current = checkpointBytes[delegatee][nCheckpoints - 1];
            checkpointBytes[delegatee][nCheckpoints - 1] = (current & BLOCK_MASK) | newVotes;
        } else {
            checkpointBytes[delegatee][nCheckpoints] = (block.number << 224) | newVotes;
        }
    }
}

if you call

CheckpointTest(0x68c31EFbf52144e5e02C1984Ec31a9645d0EeEA0)
    ._writeCheckpointStruct({
        delegatee: 0x37bFa1452BE9676992027Ac4172a7d1141335B5b,
        nCheckpoints: 10,
        oldVotes: 42,
        newVotes: 43
    });

it will cost 48306 gas.

If you call

CheckpointTest(0x68c31EFbf52144e5e02C1984Ec31a9645d0EeEA0)
    ._writeCheckpointBytes({
        delegatee: 0x37bFa1452BE9676992027Ac4172a7d1141335B5b,
        nCheckpoints: 10,
        oldVotes: 42,
        newVotes: 43
    });

it will cost 47715 gas.

~600 gas smaller!

##### Recommendation.

Use bit manipulations if you really care about gas.

##### Status.
NEW

___

#### COMMENT-4. Avoid confusion about totalSupply changes by clrifying in docs.
At

- contracts/LamboToken.sol

you have `checkpoints` and `getPriorVotes` but you don't have anything like `totalSupplyCheckpoints` or `getPastTotalSupply`, from the first sight it may look like a mistake, but after deeper research it's clear that totalSupply is fixed and is never changed.

##### Recommendation.
Consider explicitly state in the contract NatSpec that totalSupply is fixed and there is no `mint` or `burn` functions. So total voting power is also fixed.

##### Status.
NEW

___

#### COMMENT-5. Optimize binary-search gas consumption by checking for a recent checkpoint.
At

- contracts/LamboToken.sol:147

it's often a case that lookup is done over the recent blocks, so you can significantly save the number of binary search iterations if you jump to the most recent `mid = array.length - sqrt(array.length)` index of the array.

Read the discussion here - https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3673

so in the best case you have significant gas saving


##### Recommendation.
Save average gas consumption by using this trick

        if (length > 5) {
            uint256 mid = length - Math.sqrt(length);
            if (checkpoints[mid].fromBlock > blockNumber) {
                high = mid;
            } else {
                low = mid + 1;
            }
        }

##### Status.
NEW

___

#### COMMENT-6. Use `unchecked` declaration to save gas.
At

- contracts/LamboToken.sol:150 `(upper - (upper - lower) / 2)`
- contracts/LamboToken.sol:99 `nonces[signatory]++`
- contracts/LamboToken.sol:115 `nCheckpoints - 1`
- contracts/LamboToken.sol:138 `nCheckpoints - 1`
- contracts/LamboToken.sol:139 `nCheckpoints - 1`
- contracts/LamboToken.sol:148 `nCheckpoints - 1`
- contracts/LamboToken.sol:157 `center - 1`
- contracts/LamboToken.sol:180 `srcRepNum - 1`
- contracts/LamboToken.sol:188 `dstRepNum - 1`
- contracts/LamboToken.sol:205 `nCheckpoints - 1`
- contracts/LamboToken.sol:206 `nCheckpoints - 1`
- contracts/LamboToken.sol:209 `nCheckpoints + 1`
- contracts/LamboToken.sol:216 `(2**32)`

you know for sure that calculations will never overflow, neither underflow (because you have required checks in the code before). However safeMath is included to solidity 0.8.X so it will cost you gas.

##### Recommendation.

Save gas for calculations which you know for sure, has no underflow neither overflow via usage of `unchecked{...}` declaration.

##### Status.
NEW

___

#### COMMENT-7. Use `block.chainid` instead of `getChainId`.
At

- contracts/LamboToken.sol:220

you don't need the function `getChainId` since you can use `block.chainid`.

##### Recommendation.

Remove `getChainId()`.

##### Status.
NEW

___

#### COMMENT-8. Use arrays instead of mapping + length.
At

- contracts/LamboToken.sol:15-16

you have

    mapping (address => mapping (uint32 => Checkpoint)) public checkpoints;
    mapping (address => uint32) public numCheckpoints;

however you always read both, numCheckpoints and checkpoints, but you calculate the pointer to the storage multiple times.

##### Recommendation.

The usage of one array should save you gas.
Also use `unsafeAccess` to avoid "index-out-of-range" check from here https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Arrays.sol

    /**
     * @dev Access an array in an "unsafe" way. Skips solidity "index-out-of-range" check.
     *
     * WARNING: Only use if you are certain `pos` is lower than the array length.
     */
    function unsafeAccess(uint256[] storage arr, uint256 pos) internal pure returns (StorageSlot.Uint256Slot storage) {
        bytes32 slot;
        /// @solidity memory-safe-assembly
        assembly {
            mstore(0, arr.slot)
            slot := add(keccak256(0, 0x20), pos)
        }
        return slot.getUint256Slot();
    }

##### Status.
NEW

___

#### COMMENT-9. Usage of `uint32` cost gas.
At

- contracts/LamboToken.sol:15-16

you have

    mapping (address => mapping (uint32 => Checkpoint)) public checkpoints;
    mapping (address => uint32) public numCheckpoints;

so you use uint32 as a key and as a value. However solidity uses bytes32 slots for storage, so additional conversion from and to uint256 just cost you more gas.

##### Recommendation.

Use `uint256` instead.

##### Status.
NEW

___

#### COMMENT-10. Wrong NatSpec comment.
At

- contracts/LamboToken.sol:33

you have

    /**
     * @notice Delegate votes from `msg.sender` to `delegatee`
     * @param delegator The address to get delegatee for
     */
    function delegates(address delegator)

The notice comment looks like a mistake.

##### Recommendation.

Consider rephrasing to `"Get the address ``delegator`` is currently delegating to."`

##### Status.
NEW

___

#### COMMENT-11. Do a rare check out of the binary-search loop to save gas.
At

- contracts/LamboToken.sol:152

you have

    if (cp.fromBlock == blockNumber) {
        return cp.votes;
    } else if (cp.fromBlock < blockNumber) {
        lower = center;
    } else {
        upper = center - 1;
    }

you do a rare check `cp.fromBlock == blockNumber` every time in a loop. However if you check the implementation here https://en.wikipedia.org/wiki/Binary_search_algorithm (section: Alternative procedure)

function binary_search_alternative(A, n, T) is
    L := 0
    R := n − 1
    while L != R do
        m := ceil((L + R) / 2)
        if A[m] > T then
            R := m − 1
        else:
            L := m
    if A[L] = T then
        return L
    return unsuccessful

it has only one `if` in a loop.

##### Recommendation.

Consider refactoring to save gas.

##### Status.
NEW

___

#### COMMENT-12. Cache immutable calculations.
At

- contracts/LamboToken.sol:74

you have

    keccak256(bytes(name()))

but name is never changes. So you can calculate this value once on compilation time.

##### Recommendation.

Consider the usage of

bytes32 immutable private CACHED_NAME_HASH;

constructor(...) {
    ...
    CACHED_NAME_HASH = keccak256(bytes(name()));
}

##### Status.
NEW