QA Report

[code423n4]

Title: Public function massUpdatePools() can be broken by a malicious token

If a malicious user adds a factory pool with a custom erc20 token where the token reverts if any of its functions are called from the MasterChef address, any time this public function is called it will revert.

Impact

The public function can be made to revert every time, which impacts the function of the protocol. I'm not sure if this is Low or Medium, so I'm submitting this as my QA report and leaving it up to the judge. Please elaborate on the rationale so I can tag future reports correctly.

Proof of Concept

    function massUpdatePools() public {
        uint length = poolInfo.length;
        for (uint _pid = 0; _pid < length; ++_pid) {
            updatePool(_pid);
        }
    }

    // Update reward variables of the given pool to be up-to-date.
    function updatePool(uint _pid) public {
        PoolInfo storage pool = poolInfo[_pid];
        if (block.number <= pool.lastRewardBlock) {
            return;
        }
        uint lpSupply = pool.depositToken.balanceOf(address(this));
        if (lpSupply == 0 || pool.allocPoint == 0) {
            pool.lastRewardBlock = block.number;
            return;
        }
        if(block.number >= endBlock) {
            pool.lastRewardBlock = block.number;
            return;
        }        

        uint multiplier = getMultiplier(pool.lastRewardBlock, block.number);
        uint concurReward = multiplier.mul(concurPerBlock).mul(pool.allocPoint).div(totalAllocPoint);
        pool.accConcurPerShare = pool.accConcurPerShare.add(concurReward.mul(_concurShareMultiplier).div(lpSupply));
        pool.lastRewardBlock = block.number;
    }

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/MasterChef.sol#L127-L154

The balanceOf() function can be made to revert every time, which means the mass update will always fail. Any individual pool with a malicious token has its own updatePool() calls broken.

Tools Used

Code inspection

Recommended Mitigation Steps

Use try-catch when iterating over all pools in a single function

[GalloDaSballo]

Judging of this finding is fairly nuanced and in a different context could be interpreted differently.

Ultimately a big deciding factor for me is that having already went through hundreds of other findings, I know that massUpdatePools is not used (a vulnerability in it of itself, for other reasons)

But because I know that, I also know that you can't DOS the entire reward system as a malicious pool with a reverting token would just brick itself.

Technically even the pool with address(0) (which afik lacks the fallback function) would cause reverts in massUpdatePool.

Either way, I believe the warden has found a very interesting type of Admin Privilege which ultimately would allow the pool math to be broken, interestingly enough the admin would need to be "generous" enough to create a stacking pool and then malicious enough to set a token that reverts (or doesn't exist)

In lack of the remove function any pool added cannot be revoked, so that should also be taken into consideration.

All in all, with a different codebase I may have evaluated this differently, but in this case the codebase has other flaws that make it so that if the admin did set the wrong pool, while there would be no way to remove it, people could still sidestep it, withdraw from it and ultimately have a new pool set up with proper reward math.

So for that reason (easily sidesteppable, main focus of DOS denied because of other vulnerabilities) I believe the finding to be very interesting, valid but ultimately of Low Severity

[GalloDaSballo]

Adding #165 will give it 3++ as the finding here is unique