code-423n4 / 2022-02-concur-findings

2 stars 0 forks source link

Improper Upper Bound Definition on the depositFeeBP #274

Closed CloudEllie closed 2 years ago

CloudEllie commented 2 years ago

Judge @GalloDaSballo has assessed item C4-002 in QA Report #198 as Medium risk. The relevant finding follows:

Impact - LOW

The add function does not have any upper or lower bounds. Values that are too large will lead to reversions in several critical functions. User funds will be locked forever.

Proof of Concept

  1. Navigate to the following contract.

https://github.com/code-423n4/2022-02-concur/blob/02d286253cd5570d4e595527618366f77627cdaf/contracts/MasterChef.sol#L97

    function add(address _token, uint _allocationPoints, uint16 _depositFee, uint _startBlock) public onlyOwner {
        require(_token != address(0), "zero address");
        uint lastRewardBlock = block.number > _startBlock ? block.number : _startBlock;
        totalAllocPoint = totalAllocPoint.add(_allocationPoints);
        require(pid[_token] == 0, "already registered"); // pid starts from 0
        poolInfo.push(
            PoolInfo({
                depositToken: IERC20(_token),
                allocPoint: _allocationPoints,
                lastRewardBlock: lastRewardBlock,
                accConcurPerShare: 0,
                depositFeeBP: _depositFee
            })
        );
        pid[_token] = poolInfo.length - 1;
    }

Tools Used

None

Recommended Mitigation Steps

Consider to define upper bound on the add function. User can pay %100 fee when the deposit into the pool.

CloudEllie commented 2 years ago

Duplicate of #242