code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

During stake or deposit, users would not be transferred the correct Concur token, when MasterChef has under-supply of it.

There is an assumption that MasterChef contract would own enough Concur tokens so as to distribute to users as reward, during deposit or withdraw. But say, due to excess user activity, MasterChef runs out of Concur tokens. All deposits & withdraws that happen after that, would have zero transfer of Concur token to the user. This will continue till the MasterChef contract is replenished again.

Proof of Concept

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

Makeshift unit test Note: Temporarily modify the private function MasterChef.safeConcurTransfer to public function, for unit test validation

//Unit Test starts
  it("MasterChef - Zero Concur balance", async function() {
    await concurToken.mint(masterChef.address, 100);
    console.log(await concurToken.balanceOf(masterChef.address), await concurToken.balanceOf(user1.address));
    await masterChef.safeConcurTransfer(user1.address, 60); // user1 is rewarded correctly.
    console.log(await concurToken.balanceOf(masterChef.address), await concurToken.balanceOf(user1.address));
    await masterChef.safeConcurTransfer(user1.address, 60); // user1 is rewarded lesser by 10.
    console.log(await concurToken.balanceOf(masterChef.address), await concurToken.balanceOf(user1.address));
    await masterChef.safeConcurTransfer(user1.address, 60); // user1 is totally not rewarded.
    console.log(await concurToken.balanceOf(masterChef.address), await concurToken.balanceOf(user1.address));
  });
//Unit Test ends

Tools Used

Manual review, & makeshift Unit test

Recommended Mitigation Steps

Minimal recommended fix

To MasterChef.safeConcurTransfer function, add the following require statement. This will atleast ensure that, when there is zero balance in MasterChef contract, the safeConcurTransfer function will not succeed.

    function safeConcurTransfer(address _to, uint _amount) private {
        uint concurBalance = concur.balanceOf(address(this));
        require(concurBalance>0, "safeConcurTransfer: balance is zero.");

GalloDaSballo commented 2 years ago

The finding is valid as in that depositors could not receive token incentives, am not fully convinced this should be of medium severity as ultimately the contract will eventually run out of tokens and as such this is a situation that has to be handled.

If anything reverting may cause the tokens to be stuck