Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QA Report #142

Open
code423n4 opened this issue Jun 3, 2022 · 1 comment
Open

QA Report #142

code423n4 opened this issue Jun 3, 2022 · 1 comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

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.

@code423n4 code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Jun 3, 2022
code423n4 added a commit that referenced this issue Jun 3, 2022
@chase-manning chase-manning added sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) labels Jun 6, 2022
@GalloDaSballo
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

3 participants