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 #173

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

QA Report #173

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

  1. Title : Comment was not the same as actual code

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/AddressProvider.sol#L112-L129

In the function of removepool(). Comment was used to said that :

     * @return `true` if successful. 

but in actual code was :

        return removed;

So it can be changed as it should be.

Tool Used

Manual Review

Another Occurances

It happen too in this code :
1.) AddressProvider.sol Lines247-261
2). LpGauge.sol Lines.48-62

  1. Title : Redundant Code _prepare

This code was redundant and it could be deleted for better code since it has

1 and 2 with the same fn()

    /**
     * @notice Same as `_prepare(bytes32,uint256,uint256)` but uses a default delay
     */
    function _prepare(bytes32 key, uint256 value) internal returns (bool) {
        return _prepare(key, value, _MIN_DELAY);
    }

Tool Used

Manual Review

  1. Title : NatSpec is incomplete

1.) File : contracts/AddressProvider.sol (Lines.77-87)

Missing @return

    /**
     * @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;
    }

2.) File : contracts/BkdLocker.sol (Lines.77-83)

Missing @return

     /**
     * @notice Lock gov. tokens.
     * @dev The amount needs to be approved in advance.
     */
    function lock(uint256 amount) external override {
        return lockFor(msg.sender, amount);
    }

3.) File : contracts/Controller.sol (Lines.78-84)

Missing @param amount

    /**
     * @notice Prepares the minimum amount of staked BKD required by a keeper
     */
    function prepareKeeperRequiredStakedBKD(uint256 amount) external override onlyGovernance {
        require(addressProvider.getBKDLocker() != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);
        _prepare(_KEEPER_REQUIRED_STAKED_BKD, amount);
    }

4.) File : contracts/Controller.sol (Lines.100-108)

Missing @param keeper

    /**
     * @notice Returns true if the given keeper has enough staked BKD to execute actions
     */
    function canKeeperExecuteAction(address keeper) external view override returns (bool) {
        uint256 requiredBKD = getKeeperRequiredStakedBKD();
        return
            requiredBKD == 0 ||
            IERC20(addressProvider.getBKDLocker()).balanceOf(keeper) >= requiredBKD;
    }

5.) File : contracts/Controller.sol (Lines.118-130)

Missing @param payer

     * @return the total amount of ETH require by `payer` to cover the fees for
     * positions registered in all actions
     */
    function getTotalEthRequiredForGas(address payer) external view override returns (uint256) {
        // solhint-disable-previous-line ordering
        uint256 totalEthRequired;
        address[] memory actions = addressProvider.allActions();
        uint256 numActions = actions.length;
        for (uint256 i; i < numActions; i = i.uncheckedInc()) {
            totalEthRequired += IAction(actions[i]).getEthRequiredForGas(payer);
        }
        return totalEthRequired;
    }

6.) File : contracts/utils/Preparable.sol (Lines.50-55)

    /**
     * @notice Same as `_prepare(bytes32,uint256,uint256)` but uses a default delay
     */
    function _prepare(bytes32 key, uint256 value) internal returns (bool) {
        return _prepare(key, value, _MIN_DELAY);
    }

7.) File : contracts/utils/Preparable.sol (Lines.74-79)

     /**
     * @notice Same as `_prepare(bytes32,address,uint256)` but uses a default delay
     */
    function _prepare(bytes32 key, address value) internal returns (bool) {
        return _prepare(key, value, _MIN_DELAY);
    }
  1. Title : Typo Comment

1.) File : contracts/BkdLocker.sol (Line.174)

invlude change to include

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

2.) File : contracts/AddressProvider.sol (Line.237)

feeze change into freeze

     * @param key Key to feeze
  1. Title : simplify the number of _MAX_SUPPLY

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/utils/CvxMintAmount.sol#L12

  uint256 private constant _MAX_SUPPLY = 100000000 * 1e18; //100 mil max supply

changed to :

uint256 private constant _MAX_SUPPLY =  1e26 //100mil
@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

GalloDaSballo commented Jun 20, 2022

Title : Comment was not the same as actual code

Disagree as true is the value returned on success
If anything just use @dev tag

Title : NatSpec is incomplete

Very confident natspec doesn't require all fields, else the compiler would warn

1.) File : contracts/BkdLocker.sol (Line.174)

Agree with typos and with the refactoring, although it won't save gas

Overall a pretty small and concise report

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