code-423n4 / 2022-05-backd-findings

0 stars 0 forks source link

QA Report #142

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

FINDINGS

TYPOS

File: BkdLocker.sol line 732

* @dev This does not invlude the gov. tokens queued for withdrawal.

invlude Instead of include

File: FeeBurner.sol line 35

receive() external payable {} // Recieve function for withdrawing from Backd ETH Pool

Recieve instead of Receive

File:AddressProvider.sol line 237

* @param key Key to feeze    

feeze Instead of freeze

NATSPEC is Incomplete

File: AddressProvider.sol line 77-87

/**
     * @notice Adds action.
     * @param action Address of action to add.
     */
    function addAction(address action) external override onlyGovernance returns (bool) {
        bool result = _actions.add(action);
        if (result) {
            emit ActionListed(action);
        }
        return result;
    }

Missing @return

File:AddressProvider.sol line 172-178

/**
     * @notice Returns the address for the given key
     */
    function getAddress(bytes32 key) public view override returns (address) {
        require(_addressKeyMetas.contains(key), Error.ADDRESS_DOES_NOT_EXIST);
        return currentAddresses[key];
    }

Missing @param key

File:AddressProvider.sol line 180-187

    /**
     * @notice Returns the address for the given key
     * @dev if `checkExists` is true, it will fail if the key does not exist
     */
    function getAddress(bytes32 key, bool checkExists) public view override returns (address) {
        require(!checkExists || _addressKeyMetas.contains(key), Error.ADDRESS_DOES_NOT_EXIST);
        return currentAddresses[key];
    }

Missing @param key

File:AddressProvider.sol line 264-268

     * @notice Execute update of `key`
     * @return New address.
     */
    function executeAddress(bytes32 key) external override returns (address) {

Missing @param key

File:AddressProvider.sol line 365-369

     * @notice returns the pool at the given index
     */
    function getPoolAtIndex(uint256 index) external view override returns (address) {
        return _tokenToPools.valueAt(index);

Missing @param index

File: StakerVault.sol line 93-98

     * @notice Registers an address as a strategy to be excluded from token accumulation.
     * @dev This should be used if a strategy deposits into a stakerVault and should not get gov. tokens.
     * @return `true` if success.
     */
    function addStrategy(address strategy) external override returns (bool) {

Missing @param strategy

Inconsistent use/impementation of the uncheck block

Throught the contracts a library is being used to handle the arithmetic operations that can never over/underflow. The library UncheckedMath.sol has been used in all for loops to wrap the the loop increment using the function uncheckedInc() as shown below

File: StakerVault.sol line 256-263

    function getStakedByActions() external view override returns (uint256) {
        address[] memory actions = addressProvider.allActions();
        uint256 total;
        for (uint256 i; i < actions.length; i = i.uncheckedInc()) {
            total += balances[actions[i]];
        }
        return total;
    }

The above usage of the library UncheckedMath.sol and the uncheckedInc() function has been implemented in all contracts apart from the following.

File:PoolMigrationZap.sol line 38-45

        for (uint256 i; i < oldPoolAddresses_.length; ) {
            migrate(oldPoolAddresses_[i]);
            unchecked {
                ++i;
            }
        }
    }

The above uses the unchecked{} block directly rather than follow the same pattern with other contracts.

I would suggest we stick to the same approach in handling the arithmetics that can never over/underflow.

GalloDaSballo commented 2 years ago

Typo + Natspec

Ack, non-critical

Inconsistent use/impementation of the uncheck block

Valid find as consistency is important

Not a fan of the extensive format when it comes to extremely basic reports, especially when they feel like they come from an automated tool and are about typos and lack of needless documentation