c4-submissions commented 11 months ago

Lines of code

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

Vulnerability details

Impact

Fee calculations depends on shareData[_id].tokenCount, which is updated AFTER doing the transfer of token. That means, if the token is an ERC777 compatible token, users can reenter the function paying, for example, less fees on a buy operation.

Proof of Concept

NOTE --> I'm gonna work on top of the buy operation, the others are kind of the same

When users call Market, function buy, the fee they are gonna pay is retrieved by calling getBuyPrice

Market, line 152

        (uint256 price, uint256 fee) = getBuyPrice(_id, _amount); // Reverts for non-existing ID

whose implementation is

Market, lines 132 to 136

    function getBuyPrice(uint256 _id, uint256 _amount) public view returns (uint256 price, uint256 fee) {
        // If id does not exist, this will return address(0), causing a revert in the next line
        address bondingCurve = shareData[_id].bondingCurve;
        (price, fee) = IBondingCurve(bondingCurve).getPriceAndFee(shareData[_id].tokenCount + 1, _amount);
    }

which depends on shareData[_id].tokenCount, used as the argument to log2 in LinearBondingCurve, function getFee.

After that, Market pulls price + fee tokens from the user and AFTER that, it updates shareData[_id].tokenCount. That means, if token is an ERC777 token with the given hooks on transfers, it can reenter the function and buy far more tokens just by paying the same amount of fees, which leads to a theft of fees from the Market

Market, lines 153 to 161

        SafeERC20.safeTransferFrom(token, msg.sender, address(this), price + fee);
        // The reward calculation has to use the old rewards value (pre fee-split) to not include the fees of this buy
        // The rewardsLastClaimedValue then needs to be updated with the new value such that the user cannot claim fees of this buy
        uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);
        // Split the fee among holder, creator and platform
        _splitFees(_id, fee, shareData[_id].tokensInCirculation);
        rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;

        shareData[_id].tokenCount += _amount;

Recommended Mitigation Steps

Just make nonReentrant those functions.

Assessed type

Reentrancy

c4-pre-sort commented 11 months ago

minhquanym marked the issue as insufficient quality report

minhquanym commented 11 months ago

OOS

c4-judge commented 11 months ago

MarioPoneder marked the issue as unsatisfactory: Out of scope

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