updatePool() and add() will not work properly for Arbitrum due to block.number

Summary

As mentioned on the contest page of sherlock, that the project is going to deploy on both mainnet and arbitrum

The lastRewardBlock variable in the add() is used to keep track of the last block number where the rewards were calculated. If the block.number is incorrect, it could result in inaccurate calculations. Therefore, it's important to ensure that the block.number is accurate before using it in this function.

Vulnerability Detail

In the add() function of MockIchiFarm.sol block.number is being used which will not work properly on arbitrum. And same is the issue with updatePool() function

uint256 lastRewardBlock = block.number; 
        totalAllocPoint += allocPoint;
        lpToken.push(_lpToken);
        addedLPs[address(_lpToken)] = true; 

        poolInfo.push(
            PoolInfo({
                allocPoint: uint64(allocPoint),
                lastRewardBlock: uint64(lastRewardBlock),
                accIchiPerShare: 0
            })

updatePool() function

  if (block.number > pool.lastRewardBlock) {
            uint256 lpSupply = lpToken[pid].balanceOf(address(this));
            if (lpSupply > 0 && totalAllocPoint > 0) {
                uint256 blocks = block.number - pool.lastRewardBlock;
                pool.accIchiPerShare += uint128(
                    ((blocks *
                        ichiPerBlock *
                        pool.allocPoint *
                        ACC_ICHI_PRECISION) / totalAllocPoint) / lpSupply
                );
            }
            pool.lastRewardBlock = uint64(block.number);

Impact

Code Snippet

https://github.com/sherlock-audit/2023-04-blueberry/blob/96eb1829571dc46e1a387985bd56989702c5e1dc/blueberry-core/contracts/mock/MockIchiFarm.sol#L139

https://github.com/sherlock-audit/2023-04-blueberry/blob/96eb1829571dc46e1a387985bd56989702c5e1dc/blueberry-core/contracts/mock/MockIchiFarm.sol#L218

Tool used

Manual Review

Recommendation

Nabeel-javaid commented 1 year ago

Escalate for 10 USDC

As I mentioned earlier, the block.number of Arbitrum can be different from the block.number of Ethereum. According to Arbitrum Docs block.number returns the most recently synced L1 block number. Once per minute the block number in the Sequencer is synced to the actual L1 block number. This means that the Blueberry protocol could calculate the rewards based on an incorrect block.number. This could lead to liquidity providers receiving less rewards than they are entitled to. It could also lead to liquidity providers receiving rewards that they are not entitled to.

sherlock-admin commented 1 year ago

Escalate for 10 USDC

As I mentioned earlier, the block.number of Arbitrum can be different from the block.number of Ethereum. According to Arbitrum Docs block.number returns the most recently synced L1 block number. Once per minute the block number in the Sequencer is synced to the actual L1 block number. This means that the Blueberry protocol could calculate the rewards based on an incorrect block.number. This could lead to liquidity providers receiving less rewards than they are entitled to. It could also lead to liquidity providers receiving rewards that they are not entitled to.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

ctf-sec commented 1 year ago

the issue is in MockIchiFarm.sol, which is not in scope of the auditing

Nabeel-javaid commented 1 year ago

MockIchiFarm

MockIchiFarm was in the scope

hrishibhat commented 1 year ago

Escalation accepted

Invalid issue Accepting this escalation because the file was included in the scope of the readme. But issues found in mock contracts of an external protocol are not valid issues in any standard smart contract audit.