Canto Application Specific Dollars and Bonding Curves for 1155s - adriro's results

Lines of code

https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/asD/src/asD.sol#L75

Vulnerability details

Summary

The implementation of the max withdrawable amount is incorrect as it divides the calculation by the wrong denominator, leading to an incorrect result and a potential denial of service due to an overflow.

Impact

In the Application Specific Dollar protocol, users mint asD tokens by depositing NOTE tokens which are forwarded to the Canto Lending Market and exchanged for cNOTE tokens. For each deposited NOTE, users are minted an equal amount of asD tokens. Users can later withdraw their NOTE by burning their asD token, which removes the underlying tokens from the Canto Lending Market by exchanging the cNOTE for NOTE and returns them to the user. Interests generated in the Canto Lending Market can be withdrawn by the owner of this specific asD token.

Owners are limited to withdraw just the interests generated from the deposited NOTE. Users of the protocol should be able to redeem any amount of asD tokens at any time, which means that there must always be enough cNOTE to cover for the total supply of asD tokens. The owner should not be able to withdraw past this limit, which is implemented in the function withdrawCarry():

https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/asD/src/asD.sol#L72-L90

72:     function withdrawCarry(uint256 _amount) external onlyOwner {
73:         uint256 exchangeRate = CTokenInterface(cNote).exchangeRateCurrent(); // Scaled by 1 * 10^(18 - 8 + Underlying Token Decimals), i.e. 10^(28) in our case
74:         // The amount of cNOTE the contract has to hold (based on the current exchange rate which is always increasing) such that it is always possible to receive 1 NOTE when burning 1 asD
75:         uint256 maximumWithdrawable = (CTokenInterface(cNote).balanceOf(address(this)) * exchangeRate) /
76:             1e28 -
77:             totalSupply();
78:         if (_amount == 0) {
79:             _amount = maximumWithdrawable;
80:         } else {
81:             require(_amount <= maximumWithdrawable, "Too many tokens requested");
82:         }
83:         // Technically, _amount can still be 0 at this point, which would make the following two calls unnecessary.
84:         // But we do not handle this case specifically, as the only consequence is that the owner wastes a bit of gas when there is nothing to withdraw
85:         uint256 returnCode = CErc20Interface(cNote).redeemUnderlying(_amount);
86:         require(returnCode == 0, "Error when redeeming"); // 0 on success: https://docs.compound.finance/v2/ctokens/#redeem
87:         IERC20 note = IERC20(CErc20Interface(cNote).underlying());
88:         SafeERC20.safeTransfer(note, msg.sender, _amount);
89:         emit CarryWithdrawal(_amount);
90:     }

Lines 75-77 calculate the maximum withdrawable amount the owner can remove. This is done by multiplying the current balance of cNOTE for the exchange rate, dividing by the normalization factor, and then subtracting the supply of asD. The intention is quite simple: we take the cNOTE balance, translate it to NOTE using the exchange rate, then subtract the total supply of asD (to account for every deposit made in the protocol), and that yields the amount entitled to the owner.

However, the applied normalization factor after the balance is multiplied by the exchange rate is incorrect. The value is not 1e28, it should be 1e18. This can be seen in the logic of Compound: let's drill down on the redeem() function which is used to exchange cTokens for the underlying token, which will let us see how to correctly convert cTokens into the underlying token. redeem() calls redeemInternal(), which ends up calling redeemFresh(payable(msg.sender), redeemTokens, 0):

https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/CToken.sol#L480-L497

480:     function redeemFresh(address payable redeemer, uint redeemTokensIn, uint redeemAmountIn) internal {
481:         require(redeemTokensIn == 0 || redeemAmountIn == 0, "one of redeemTokensIn or redeemAmountIn must be zero");
482: 
483:         /* exchangeRate = invoke Exchange Rate Stored() */
484:         Exp memory exchangeRate = Exp({mantissa: exchangeRateStoredInternal() });
485: 
486:         uint redeemTokens;
487:         uint redeemAmount;
488:         /* If redeemTokensIn > 0: */
489:         if (redeemTokensIn > 0) {
490:             /*
491:              * We calculate the exchange rate and the amount of underlying to be redeemed:
492:              *  redeemTokens = redeemTokensIn
493:              *  redeemAmount = redeemTokensIn x exchangeRateCurrent
494:              */
495:             redeemTokens = redeemTokensIn;
496:             redeemAmount = mul_ScalarTruncate(exchangeRate, redeemTokensIn);
497:         } else {

When redeemTokensIn > 0 (our case) the implementation calls mul_ScalarTruncate() using the exchange rate and the amount of cTokens in order to obtain the amount of underlying tokens. mul_ScalarTruncate() will multiply both numbers and then call truncate() on the product, which divides by the expScale factor.

37:     function mul_ScalarTruncate(Exp memory a, uint scalar) pure internal returns (uint) {
38:         Exp memory product = mul_(a, scalar);
39:         return truncate(product);
40:     }

https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/ExponentialNoError.sol#L116-L118

116:     function mul_(Exp memory a, uint b) pure internal returns (Exp memory) {
117:         return Exp({mantissa: mul_(a.mantissa, b)});
118:     }

136:     function mul_(uint a, uint b) pure internal returns (uint) {
137:         return a * b;
138:     }

https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/ExponentialNoError.sol#L29-L32

29:     function truncate(Exp memory exp) pure internal returns (uint) {
30:         // Note: We are not using careful math here as we're performing a division that cannot fail
31:         return exp.mantissa / expScale;
32:     }

Since expScale is 1e18, the operation to calculate the amount of underlying tokens given an amount of cTokens is effectively cTokens * exchangeRate / 1e18.

This means that the normalization factor in the calculation of maximumWithdrawable is off by 1e10, causing the calculation not only to be incorrect, but to likely cause a denial of service due to a math overflow when subtracting the total supply. Since the number is being divided by a bigger amount than should be, the implementation will end up subtracting a larger number, causing an overflow due to the result being negative.

Proof of Concept

To simplify the demonstration and better show the issue, we will mimic the operations that are done internally in withdrawCarry() of the asD contract.

In the scenario, we have 100e18 total supply of asD tokens, and we also assume the contract holds the same quantity of cNOTE tokens. We can think of this as if the exchange rate was 1=1 when the NOTE was deposited in the Canto Lending Market. The test forks the Canto network to test against the real current exchange rate of the deployed Canto Lending Market.

When the test executes the calculation in the commented line (cNoteBalance * exchangeRate) / 1e28 - totalSupply, it will revert due to an arithmetic overflow.

When dividing by 1e18, the calculation returns the correct amount, that aligns perfectly when redeeming all the cNOTE tokens and comparing it to the total supply of asD, which represents the NOTE balance owned by depositors of the protocol (maximumWithdrawable == noteBalance - totalSupply).

The test should be executed by forking the Canto network:

forge test -vvv --mc AuditTest --fork-url=wss://canto.gravitychain.io:8546

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_withdrawCarry_IncorrectMaxWithdrawable() public {
    CTokenInterface cNote = CTokenInterface(CNOTE);
    IERC20 note = IERC20(NOTE);

    // Simulate we are operating under the asD contract
    address asD = makeAddr("asD");

    // Let's assume the total supply in asD is 100e18 (meaning we minted on 100e18 $NOTE)
    uint256 totalSupply = 100e18;
    // And let's assume we have 100e18 cNote tokens
    deal(CNOTE, asD, 100e18);

    // Log cNOTE balance (should be 100e18)
    uint256 cNoteBalance = cNote.balanceOf(asD);
    console.log("Initial cNote balance:", cNoteBalance);

    // Calc maxWithdraw
    uint256 exchangeRate = CTokenInterface(cNote).exchangeRateCurrent();
    // The following will overflow! due to the division of 1e28 instead of 1e18
    // uint256 maximumWithdrawable = (cNoteBalance * exchangeRate) / 1e28 - totalSupply;
    uint256 maximumWithdrawable = (cNoteBalance * exchangeRate) / 1e18 - totalSupply;
    console.log("Correct maximumWithdrawable:", maximumWithdrawable);

    // asD redeems all cNote
    vm.prank(asD);
    CErc20Interface(CNOTE).redeem(cNoteBalance);

    // Log NOTE balance
    uint256 noteBalance = note.balanceOf(asD);
    console.log("Note balance after redeem:", noteBalance);

    // Check the maximumWithdrawable is ok
    assertEq(maximumWithdrawable, noteBalance - totalSupply);
}

Recommendation

The calculation of maximumWithdrawable in withdrawCarry() should divide by 1e18 instead of 1e28.

        uint256 maximumWithdrawable = (CTokenInterface(cNote).balanceOf(address(this)) * exchangeRate) /
-           1e28 -
+           1e18 -
            totalSupply();

Assessed type

Other

Lines of code

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

Vulnerability details

Summary

When tokens are sold, a percentage of the fees go to each token holder, including the seller. If a seller sells tokens in batch, they will get more in fees since each one of the tokens being sold is still counted as a token the user holds.

Impact

By design, tokens sold in the 1155tech Market distribute fees to token holders, including the same holder who is selling the tokens. The behavior is present in the sell() function:

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

174:     function sell(uint256 _id, uint256 _amount) external {
175:         (uint256 price, uint256 fee) = getSellPrice(_id, _amount);
176:         // Split the fee among holder, creator and platform
177:         _splitFees(_id, fee, shareData[_id].tokensInCirculation);
178:         // The user also gets the rewards of his own sale (which is not the case for buys)
179:         uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);
180:         rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;
181: 
182:         shareData[_id].tokenCount -= _amount;
183:         shareData[_id].tokensInCirculation -= _amount;
184:         tokensByAddress[_id][msg.sender] -= _amount; // Would underflow if user did not have enough tokens
185: 
186:         // Send the funds to the user
187:         SafeERC20.safeTransfer(token, msg.sender, rewardsSinceLastClaim + price - fee);
188:         emit SharesSold(_id, msg.sender, _amount, price, fee);
189:     }

Line 177 splits the fees, which distribute the holder share of the fee between all the token holders (fees are divided by tokensInCirculation). Seller tokens are later decremented in line 184. This means that fees generated from the sell also apply to the seller, which is stated by the comment in line 178:

// The user also gets the rewards of his own sale (which is not the case for buys)

However, this implementation contains an issue where a seller that sells tokens in batch will get more fees than a seller who sells one by one, or in smaller chunks.

When a shareholder sells N tokens, they will benefit from collecting fees from all N tokens. This happens because the operation is done in batch and the fees will be distributed before the N tokens are reduced from tokensByAddress. When the same operation is done one by one, the first sell will be distributed between N tokens, but the next sell will be distributed among N-1 tokens, because the previous operation already reduced tokensByAddress, then N-2, and so on.

Proof of Concept

We demonstrate the issue in the following test. We mock two identical shares to show the issue as a difference between both scenarios, a seller who sells in batch and a seller who sells the same tokens, at the same time, at the same prices, but one by one.

shareId1 and shareId2 are two different shares with the same parameters and bonding curve. First we bootstrap both shares by making Mary buy the same amount of shares in each one. Then, in share1 Alice purchases 10 tokens and then sells them in batch by calling market.sell(shareId1, 10). After the operation, she is left with 993384333333333343 payment tokens. Charlie, purchases the same amount of tokens from share2, but he does 10 calls to market.sell(shareId2, 1), yielding 993188166666666670. Now, both have purchased the same amount of tokens and sold the same amount of tokens from shares that are identical, but Alice has 196166666666673 more than Charlie.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_Sell_FeeDifferenceInBatch() public {
    market.changeBondingCurveAllowed(address(bondingCurve), true);
    market.restrictShareCreation(false);

    // Create two shares
    vm.prank(bob);
    uint256 shareId1 = market.createNewShare("Test Share 1", address(bondingCurve), "metadataURI");
    vm.prank(bob);
    uint256 shareId2 = market.createNewShare("Test Share 2", address(bondingCurve), "metadataURI");

    // setup alice, charlie and mary with 1e18 tokens each
    uint256 initialAmount = 1e18;
    deal(address(token), alice, initialAmount);
    deal(address(token), charlie, initialAmount);
    deal(address(token), mary, initialAmount);

    // Mary buys some initial tokens on both shares
    vm.startPrank(mary);

    token.approve(address(market), type(uint256).max);

    market.buy(shareId1, 5);
    market.buy(shareId2, 5);

    vm.stopPrank();

    // Alice sells 10 tokens in one call
    vm.startPrank(alice);

    token.approve(address(market), type(uint256).max);

    market.buy(shareId1, 10);
    market.sell(shareId1, 10);

    market.claimHolderFee(shareId1);

    uint256 aliceFinalBalance = token.balanceOf(alice);
    console.log("Alice balance:", aliceFinalBalance);

    vm.stopPrank();

    // Charlie does 10 sells of 1 token
    vm.startPrank(charlie);

    token.approve(address(market), type(uint256).max);

    market.buy(shareId2, 10);

    for (uint256 index = 0; index < 10; index++) {
        market.sell(shareId2, 1);
    }

    market.claimHolderFee(shareId2);

    uint256 charlieFinalBalance = token.balanceOf(charlie);
    console.log("Charlie balance:", charlieFinalBalance);

    vm.stopPrank();

    // Alice got more after selling the same amount
    uint256 difference = aliceFinalBalance - charlieFinalBalance;
    assertGt(difference, 0);
    console.log("Difference:", difference);
}

Recommendation

Selling tokens in batch should yield the same fees as selling them one by one or in smaller batches. When selling more than 1 token, the fees from each of the tokens being sold need to account for other tokens sold in the batch, so that fees are properly accounted for in both cases.

Note from warden

Although similar, this is a different scenario than the one mentioned in the contest documentation:

NFT minting / burning fees are based on the current supply. This leads to the situation that buying 100 tokens and then minting 100 NFTs is more expensive than buying 1, minting 1, buying 1, minting 1, etc... (100 times). We do not consider this a problem because a user typically has no incentives to mint more than one NFT.

Assessed type

Other