c4-submissions commented 10 months ago

Lines of code

https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/1155tech-contracts/src/Market.sol#L171-L189

Vulnerability details

Proof of Concept

Take a look at Market.sol#L171-L189

    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
        //@audit-issue
        SafeERC20.safeTransfer(token, msg.sender, rewardsSinceLastClaim + price - fee);
        emit SharesSold(_id, msg.sender, _amount, price, fee);
    }

As seen, the function is used to sell any amounts of tokens for a given share ID, and the seller receives the funds gotten from this sale minus the fees.

Now, protocol implements allow/disallow listing mechanism even for Market.sol which means that they should be able to deal with ERC20 tokens that also deal with this, note that it's been stated that other ERC20 tokens should be able to work with this contract, i.e 1155tech currently uses asD but it could use any token, since asD is a stablecoin we should expect that this contract complies well with even if not all stable coins out there, at least some of them, and the argument should hold the strongest for the most adopted ones out there.

USDC and USDT are the most adopted stablecoins in the market, both currently amounting to 110 billion + market cap, now issue is that both USDC/USDT have a blocklisting mechanism where they can add any address to this list and as such stop interactions with their tokens from this address, using USDC as a case study, its got a notBlacklisted() modifier that's required for functions such as transfer(), transferFrom() and approve().

In this protocol's specific scenario, if a user's address gets blacklisted by the underlying ERC20 token, then this line: SafeERC20.safeTransfer(token, msg.sender, rewardsSinceLastClaim + price - fee); reverts, since the receiver of these tokens have been hardcoded to be the msg.sender, essentially leading to msg.sender not being able to call sell() for any of their tokens. transaction to revert and thus locking the user's funds in the contract.

Impact

Users inability to sell any amount of a given share ID (could as well be all available share Ids, just depends on what token is used as an underlying).
The above leads to user frustration, if a user is using a DCA strategy they also can't go ahead with this.
If market is not really favourable and users would like to do away with their tokens, they can't do that which would also lead to loss in $US value of their assets.

Where as users assets are somewhat at risk, I'm submitting this as medium seveerity finding, cause it has like a hypothetical attack path, additionally the availability of one of the protocol's core functionality is defiinitely impacted which is solid ground for medium based on Code4rena's Severity Categorization.

Tool Used

Manual Review

Recommended Mitigation Steps

A popular approach of fixing this bug case is to allow user to provide a fresh address, i. in this case, there should be provisions for the seller to provide a fresh address while trying to sell any amount of any given share Id.

-    function sell(uint256 _id, uint256 _amount) external {
+    function sell(uint256 _id, uint256 _amount, address receiver) 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);
+        SafeERC20.safeTransfer(token, receiver, rewardsSinceLastClaim + price - fee);
        emit SharesSold(_id, msg.sender, _amount, price, fee);
    }

Assessed type

Token-Transfer

c4-pre-sort commented 10 months ago

minhquanym marked the issue as insufficient quality report

minhquanym commented 10 months ago

Blacklist token, consider QA

c4-judge commented 9 months ago

MarioPoneder changed the severity to QA (Quality Assurance)

MarioPoneder commented 9 months ago

Not a bug of the protocol itself, but good QA recommendation.

c4-judge commented 9 months ago

MarioPoneder marked the issue as grade-b

Bauchibred commented 9 months ago

Hi @MarioPoneder, thanks for judging, but I'd like to disagree with the fact that you consider this not a bug of the protocol itself and would appreciate if you could have another look.

This is just like every other protocol hardcoded issues with this case being hardcoded receiver addresses, note that in some areas, this bug case leads to a complete loss of the tokens say in the case where protocol is a multi chain project and hardcodes the address to receive a token to msg.sender while making interchain transfers, or say a DOS for a classic case like when a protocol hardcodes the fee for every uniswap pool expecting all tokens to have a pool existing (or even enough liquidity) with that fee, where as the aforementioned cases is not in direct relation to the above report, it proves that hardcoding the receiver address should be considered a bug within the protocol.

Additionally, based on Code4rena's severity categorization, quoting "the function of the protocol or its availability could be impacted", that's in this case the fact that the Market.sell() function would be completely unavailable to some users & since this has a hypothetical path to the loss/stuckage, one can argue with these two facts, rightfully so, that this is a medium severity finding.

Lastly, do note that this is exactly the same bug case as was reported in the Maia contest from Code4rena late Q3/early Q4 2023 which was validated as a medium, coupling this with the fact that the market share of these type of tokens is even getting larger (with USDC & USDT being at the forefront since they are the most adopted stablecoins in the market) and the crypto bull run somewhat kicking in, I believe this is in fact a medium severity finding.

Thank you for your time.

MarioPoneder commented 9 months ago

Thank you for your comment!

I definitely appreciate your point of view on this, it's valuable input.
My point of view is the following, the likelihood of this scenario is very low since addresses don't just get accidentally blacklisted by the operators of USDC/USDT, there typically is a legal background story in order for this to happen.
If the sponsor implemented this report's recommendations, they could help to avoid sanctions.

code-423n4 / 2023-11-canto-findings

A user could be DOS'd from selling any share tokens of a `share ID` for an indefinite timeframe, potentially forever #276