search

code-423n4 / 2024-07-munchables-findings

6 stars 1 forks source link

If finalBonus is negative LandManager::_farmPlots() will revert due to multiplication which results in a number greater than type(uint256).max #2

Closed c4-bot-2 closed 3 months ago

c4-bot-2 commented 3 months ago

Lines of code

https://github.com/code-423n4/2024-07-munchables/blob/main/src/managers/LandManager.sol#L232-L310

Vulnerability details

Impact

In the LandManager.sol contract after the first time users stake their NFT for a certain plot via the stakeMunchable() function, and later on they try to unstake, transfer their NFT to a different plot, or simple decide to farm, the _farmPlots() function will be called: 

    function _farmPlots(address _sender) internal {
        (address mainAccount, MunchablesCommonLib.Player memory renterMetadata) = _getMainAccountRequireRegistered(_sender);

        uint256[] memory staked = munchablesStaked[mainAccount];
        MunchablesCommonLib.NFTImmutableAttributes memory immutableAttributes;
        ToilerState memory _toiler;
        uint256 timestamp;
        address landlord;
        uint256 tokenId;
        int256 finalBonus;
        uint256 schnibblesTotal;
        uint256 schnibblesLandlord;
        for (uint8 i = 0; i < staked.length; i++) {
            timestamp = block.timestamp;
            tokenId = staked[i];
            _toiler = toilerState[tokenId];
            if (_toiler.dirty) continue;
            landlord = _toiler.landlord;
            ...
            (,MunchablesCommonLib.Player memory landlordMetadata) = _getMainAccountRequireRegistered(landlord);

            immutableAttributes = nftAttributesManager.getImmutableAttributes(tokenId);
            finalBonus =
                int16(REALM_BONUSES[(uint256(immutableAttributes.realm) * 5) + uint256(landlordMetadata.snuggeryRealm)]) +
                int16(int8(RARITY_BONUSES[uint256(immutableAttributes.rarity)]));
            schnibblesTotal = (timestamp - _toiler.lastToilDate) * BASE_SCHNIBBLE_RATE;
            /// INFO: If final bonus is a negative number schibblesTotal will underflow
            schnibblesTotal = uint256((int256(schnibblesTotal) + (int256(schnibblesTotal) * finalBonus)) / 100);

            ...
        }
        accountManager.updatePlayer(mainAccount, renterMetadata);
    }

As can be seen from the above code snippet finalBonus is of type int256 thus it is possible for it to be a negative number. As can be seen from the deployment scripts REALM_BONUSES there are several values that are negative. The RARITY_BONUSES are as follows: [0, 0, 10, 20, 30, 50]. If for example immutableAttributes.realm is 1, landlordMetadata.snuggeryRealm is 0, and immutableAttributes.rarity is 0 or 1 finalBonus will be -10. When the _farmPlots() function tries to calculate schnibblesTotal the value will be a very big number close to type(uint256).max. In the next line when the contract tries to calculate the schnibblesLandlord.

schnibblesLandlord = (schnibblesTotal * _toiler.latestTaxRate) / 1e18;

The call will revert as the _toiler.latestTaxRate is in the range of 20e16 to 75e16 according to the deployment scripts. And calculating *schnibblesTotal _toiler.latestTaxRate will results in a number that is greater than type(uint256).max**. There are a couple of impacts:

Proof of Concept

Gist After follwing the steps in the above mentioned gist add the following test to the AuditorTests.t.sol file:

    function test_NegativeFinalBonusRevertsPlotFarming() public {
        mintNFT(alice);

        vm.startPrank(landLord1);
        /// @notice realm is equal to 0
        accountManagerContract.register(MunchablesCommonLib.Realm.Everfrost, address(0));
        deal(landLord1, 1e18);
        lockManager.lock{value: 1e18}(address(0), 1e18);
        uint256 landLord1lockedWeight = lockManager.getLockedWeightedValue(landLord1);
        console2.log("Available plots: ", landLord1lockedWeight / 30e18);
        vm.stopPrank();

        vm.startPrank(alice);
        accountManagerContract.register(MunchablesCommonLib.Realm.Drench, address(0));
        munchNFT.approve(address(landManagerContract), 1);
        landManagerContract.stakeMunchable(landLord1, 1, 1);

        /// @notice skip 1 hour
        skip(3600);
        vm.expectRevert(stdError.arithmeticError);
        landManagerContract.farmPlots();
        vm.stopPrank();
    }
Logs:
  Available plots:  125

To run the test use: forge test -vvv --mt test_NegativeFinalBonusRevertsPlotFarming

Tools Used

Manual review & Foundry

Recommended Mitigation Steps

Consider checking whether finalBonus is a negative number, and if so don't calculate any schnibbles rewards, only update the other variables updated in the _farmPlots() function. If schnibbles rewards are supposed to be negative consider a utilizing a library that supports 512 bit multiplication. However additional precautions must be taken so that 

landlordMetadata.unfedSchnibbles += schnibblesLandlord;
landlordMetadata.lastPetMunchable = uint32(timestamp);
accountManager.updatePlayer(landlord, landlordMetadata);
accountManager.updatePlayer(mainAccount, renterMetadata);

don't cause the function to revert as well.

Assessed type

Under/Overflow

0xinsanity commented 3 months ago

this is solved by the math fix: https://github.com/munchablesorg/munchables-common-core-latest/pull/55

0xinsanity commented 3 months ago

Repeat

c4-judge commented 3 months ago

alex-ppg marked the issue as duplicate of #80

c4-judge commented 3 months ago

alex-ppg marked the issue as partial-75

AtanasDimulski commented 3 months ago

Hey @alex-ppg, thanks for the swift judging. This issue describes the exact scenario for the attack outlined in #80, provides a POC, and identifies the same impact. While I may argue that this report is better than the primary one,(it was selected as such before) I understand that selecting the best report is a subjective task. However, I don't agree that this report deserves a partial-75 credit, this report is of high quality. Especially considering reports such as #263 have full credit. I urge you to give this report full credit. Thanks for your time!

alex-ppg commented 2 months ago

Hey @AtanasDimulski, thank you for your PJQA contribution. Per C4 guidelines an inadequate or absent alleviation is grounds for penalization, and this submission does not contain a correct alleviation.

Additionally, this submission was never the primary one as it was incorrectly set as a unique one (i.e. alone) and was re-duplicated under #80.

AtanasDimulski commented 2 months ago

Thanks for taking the time to reply! Also, I want to thank you for being fair and upholding the same high standard for all the reports in this contest. Thank you for your time!