badger-avatars-review-2.MD

Med

## Trust in the keeper This check is always true, even for upkeeps that governance did not set https://github.com/Badger-Finance/badger-avatars/blob/bf50895aa9ce82a261170153e1d2c8f83bd55692/src/registry/AvatarRegistry.sol#L323-L326

            require(
                avatarsInfo[avatarTarget].upKeepId == 0,
                "AvatarRegistry: UpKeep already register!"
            );

A check for name to be non-zero or status being TESTING && upkeepId == 0 would be better to avoid registering random / untrusted upkeeps https://github.com/Badger-Finance/badger-avatars/blob/bf50895aa9ce82a261170153e1d2c8f83bd55692/src/registry/AvatarRegistry.sol#L174-L179

        avatarsInfo[avatarAddress] = AvatarInfo({
            name: name,
            gasLimit: gasLimit,
            status: AvatarStatus.TESTING,
            upKeepId: 0
        });

I also think just immediately registering via governance would help simplify the code

QA

Filtering by Status

See this POC for how to trim the lenght of array

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.13;

import "../../lib/test.sol";
import "../../lib/Console.sol";

contract Exploit is DSTest {
    StatusExample testC;

    function testExploit(uint256 nonce) public {
        testC = new StatusExample();
        
        uint256[] memory temp = testC.getByStatus(3);
        assertEq(temp.length, 2);
        assertEq(temp[0], 4);
        assertEq(temp[1], 5);
    }
}

contract StatusExample {

    uint[] internal avatars;

    constructor() {
        avatars.push(1);
        avatars.push(2);
        avatars.push(3);
        avatars.push(4);
        avatars.push(5);
    }
    function getByStatus(uint256 minValue) external returns (uint256[] memory) {
        uint256 length = avatars.length;
        uint256 amounts;
        uint256[] memory temp = new uint256[](length);

        // Fill array, end is empty
        for(uint256 i; i < length; i++) {
            // Cache to skip second SLOAD
            uint256 currentVal = avatars[i];
            if(currentVal > minValue) {
                temp[amounts] = currentVal;
                unchecked { ++amounts; }   
            }
        }

        // Trim array
        uint256[] memory secondTemp = new uint256[](amounts);
        for(uint256 i; i < amounts; i++) {
            secondTemp[i] = temp[i];
        }

        return secondTemp;
    }
}

Can quickly test it via this repo: https://github.com/0xKitsune/gas-lab Ultimately you need to create two arrays as I'm not aware of the possibility of removing length from an already create one

Unused Variables Refactoring

https://github.com/Badger-Finance/badger-avatars/blob/bf50895aa9ce82a261170153e1d2c8f83bd55692/src/registry/AvatarRegistry.sol#L516-L517

        (state, _c, _k) = CL_REGISTRY.getState();

Can be changed to

        (state, , ) = CL_REGISTRY.getState();

To ignore them and not store them in the stack

Gas

Make Immutable

https://github.com/Badger-Finance/badger-avatars/blob/bf50895aa9ce82a261170153e1d2c8f83bd55692/src/registry/AvatarRegistry.sol#L65-L66

    address public governance;

Notes

https://github.com/Badger-Finance/badger-avatars/blob/bf50895aa9ce82a261170153e1d2c8f83bd55692/src/registry/AvatarRegistry.sol#L147-L148

            LINK.approve(KEEPER_REGISTRY, type(uint256).max); // NOTE

Safe because

  /**
   * @dev Approve the passed address to spend the specified amount of tokens on behalf of msg.sender.
   * @param _spender The address which will spend the funds.
   * @param _value The amount of tokens to be spent.
   */
  function approve(address _spender, uint256 _value) returns (bool) {
    allowed[msg.sender][_spender] = _value;
    Approval(msg.sender, _spender, _value);
    return true;
  }