c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-11-canto/blob/main/1155tech-contracts/src/Market.sol#L263-L270

Vulnerability details

Impact

Non Holder can Claim Holder Fee before the actual Authorized holder can claim it causing loss of fund to the authorized holder in the Market.sol contract.

Proof of Concept

  function claimHolderFee(uint256 _id) external {
        uint256 amount = _getRewardsSinceLastClaim(_id);
        rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;
        if (amount > 0) {
            SafeERC20.safeTransfer(token, msg.sender, amount);
        }
        emit HolderFeeClaimed(msg.sender, _id, amount);
    }

As seen from the claimHolderFee(...) function implementation above, no validation is done at any point in time in the function to ensure only authorized Holders can call the function. Most Importantly As noted from the Protocol Contest and Code base Description : "Claiming : The functions claimPlatformFee, claimHolderFee, and claimCreatorFee are used by the platform team, holders, and creators to claim the accrued fees." This shows that claiming any form of fee or reward should not be callable by unauthorized parties.

In addition it can be noted that at L184 and other functions of this contract and as provided below, underflow seems to be used as the strategy to revert the claim reward if user does not previously have enough balance to be claimed i.e has no right to claim reward

 function sell(uint256 _id, uint256 _amount) external {
        (uint256 price, uint256 fee) = getSellPrice(_id, _amount);
        // Split the fee among holder, creator and platform
        _splitFees(_id, fee, shareData[_id].tokensInCirculation);
        // The user also gets the rewards of his own sale (which is not the case for buys)
        uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);
        rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;

        shareData[_id].tokenCount -= _amount;
        shareData[_id].tokensInCirculation -= _amount;
>>>      tokensByAddress[_id][msg.sender] -= _amount; // Would underflow if user did not have enough tokens

        // Send the funds to the user
        SafeERC20.safeTransfer(token, msg.sender, rewardsSinceLastClaim + price - fee);
        emit SharesSold(_id, msg.sender, _amount, price, fee);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Necessary state and modifier should be created for the code base and used as access control for the claimHolderFee(...) function.

+++ modifier onlyHolder() {
+++        require(
+++             msg.sender == HolderState,
+++            "Not Holder"
+++        );
+++        _;
+++    }

  function claimHolderFee(uint256 _id) external onlyHolder {
        uint256 amount = _getRewardsSinceLastClaim(_id);
        rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;
        if (amount > 0) {
            SafeERC20.safeTransfer(token, msg.sender, amount);
        }
        emit HolderFeeClaimed(msg.sender, _id, amount);
    }

Note: the mitigation would be more complex than this but this is just to show an idea of how the mitigation would look like. A second Possible approach is to use the same underflow technique as noted in the code in this report which takes advantage of the value of "tokensByAddress[_id][msg.sender]" to determine if msg.sender is allowed to claim Holder fee