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

[QA-01] It's possible to create multiple of asD tokens with the same name and symbol

File: asDFactory.sol

function create(string memory _name, string memory _symbol) external returns (address) {
        asD createdToken = new asD(_name, _symbol, msg.sender, cNote, owner());
        isAsD[address(createdToken)] = true;
        emit CreatedToken(address(createdToken), _symbol, _name, msg.sender);
        return address(createdToken);
    }

Function create() does not verify if token with the same name and symbol already exist. Since function is external, anyone can call it and create own token. Allowing creating multiple of different tokens with the same name and symbol may be misleading to the end user and may increase number of scams (malicious actors will be creating the same tokens with different addresses).

Our recommendation is to verify if asD token with the same name and symbol already exists.

[QA-02] Lack of 0-check in LinearBondingCurve.sol in constructor()

Whenever contract is deployed with priceIncrease = 0, function getPriceAndFee() will always return 0.

File: LinearBondingCurve.sol

 for (uint256 i = shareCount; i < shareCount + amount; i++) {
            uint256 tokenPrice = priceIncrease * i;
            price += tokenPrice;
            fee += (getFee(i) * tokenPrice) / 1e18;
        }

Since priceIncrease defines how much the price increases per share, whenever this value is set to 0, both tokenPrice and fee will be 0 (since in both cases, we multiply by priceIncrease).

[QA-03] Inconsistency in documentation about share creation

According to documentation, share creation can be: "open for all or only for some whitelisted addresses".

Whenever share creation is restricted, only whitelisted addresses should be able to create new shares. However, onlyShareCreator() modifier demonstrates, that when share creation is restricted, both whitelisted addresses AND the owner can create new shares:

File: Market.sol

 modifier onlyShareCreator() {
        require(
            !shareCreationRestricted || whitelistedShareCreators[msg.sender] || msg.sender == owner(),
            "Not allowed"
        );
        _;
    }

Update the documentation, to let the users know, that when share creation is disabled, both whitelisted addresses and the owner can create new shares.

[QA-04] changeShareCreatorWhitelist() does not emit an event

Function changeShareCreatorWhitelist adds or removes an address from the whitelist of share creator. However, it does not emit an event, whenever address is being added/removed from the whitelist

File: Market.sol

    function changeShareCreatorWhitelist(address _address, bool _isWhitelisted) external onlyOwner {
        require(whitelistedShareCreators[_address] != _isWhitelisted, "State already set");
        whitelistedShareCreators[_address] = _isWhitelisted;
    }

[QA-05] Unfair fee split in Market.sol

File: Market.sol

 if (_tokenCount > 0) {
            shareData[_id].shareHolderRewardsPerTokenScaled += (shareHolderFee * 1e18) / _tokenCount;
        } else {
            // If there are no tokens in circulation, the fee goes to the platform
            platformFee += shareHolderFee;
        }

Whenever there's no tokens in circulation, the whole fee goes to the platform. This favors platform over share creators, especially, for the first buys (when there's no tokens in circulations yet). To balance the fee - our recommendation is to split the remaining fee (shareHolderFee) between share creator and platform:

        if (_tokenCount > 0) {
            shareData[_id].shareHolderRewardsPerTokenScaled += (shareHolderFee * 1e18) / _tokenCount;
        } else {
            
            uint feeToShare = shareHolderFee / 2;
            platformFee += feeToShare + feeToShare % 2; // % 2 to get the remaining 1, when feeToShare is not even
            shareData[_id].shareCreatorPool += feeToShare;
        }

[N-01] Use constant for CSR address

To improve the code readability - it would be better to define CSR address as constant variable.

File: asD.sol

Turnstile turnstile = Turnstile(0xEcf044C5B4b867CFda001101c617eCd347095B44);

File: asDFactory.sol

Turnstile turnstile = Turnstile(0xEcf044C5B4b867CFda001101c617eCd347095B44);

File: Market.sol

Turnstile turnstile = Turnstile(0xEcf044C5B4b867CFda001101c617eCd347095B44);

[N-02] Do not repeat the same block of code in multiple of files

        if (block.chainid == 7700 || block.chainid == 7701) {
            // Register CSR on Canto main- and testnet
            Turnstile turnstile = Turnstile(0xEcf044C5B4b867CFda001101c617eCd347095B44);
            turnstile.register(_csrRecipient);
        }

The code-block responsible for registering CSR on Canto main-net or testnet is used in multiple of files: asD.sol, asDFactory.sol, Market.sol.

It will improve the code readability - when you implement this functionality as a function and call that function in above files.

[N-03] Redundant returnCode variable in asD.sol

52:        uint256 returnCode = cNoteToken.mint(_amount);
53:        // Mint returns 0 on success: https://docs.compound.finance/v2/ctokens/#mint

63:        uint256 returnCode = cNoteToken.redeemUnderlying(_amount); // Request _amount of NOTE (the underlying of cNOTE)
64:        require(returnCode == 0, "Error when redeeming"); // 0 on success: https://docs.compound.finance/v2/ctokens/#redeem-underlying

85:        uint256 returnCode = CErc20Interface(cNote).redeemUnderlying(_amount);
86:        require(returnCode == 0, "Error when redeeming"); // 0 on success: https://docs.compound.finance/v2/ctokens/#redeem

Below code-snippets quote Compound documentation. In the documentation, the returnCode is used straightforwardly, e.g.:

require(cToken.redeemUnderlying(50) == 0, "something went wrong");

This implies, that declaration of returnCode is redundant.

While this issue should be reported in Gas report (redundant variable declaration saves gas) - I put this issue in QA Report. Code-base straightforwardly points out to the Compound docs. Those docs provides straightforward examples of calling and checking return code for mint() and redeemUnderlying(). For the code readability - the return code should be checked in the same way as it's done in the docs - without declaring additional variable.

[N-04] Interpunction in asD.sol comments

File: asD,sol

/// @dev The function checks that the owner does not withdraw too much NOTE, i.e. that a 1:1 NOTE:asD exchange rate can be maintained after the withdrawal

There should be comma after i.e. - i.e., - reference

[N-05] Use constants instead of hard-coded numbers

Using constant instead of hard-coded numbers (such as: 1e28, 1e17, 1e18, 10_000), will improve code readability.

File: asD.sol

1e28 - 

File: LinearBondingCurve.sol

fee += (getFee(i) * tokenPrice) / 1e18;

File: LinearBondingCurve.sol

 return 1e17 / divisor;

File: Market.sol

1e18;

File: Market.sol

shareData[_id].shareHolderRewardsPerTokenScaled += (shareHolderFee * 1e18) / _tokenCount;

File: Market.sol

fee = (priceForOne * _amount * NFT_FEE_BPS) / 10_000;

File: Market.sol

        uint256 shareHolderFee = (_fee * HOLDER_CUT_BPS) / 10_000;
        uint256 shareCreatorFee = (_fee * CREATOR_CUT_BPS) / 10_000;

[N-06] Incorrect comment in Market.sol

File: Market.sol

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

"Would underflow" should be changed to "would revert when underflow happens". Current comment might suggest that this might underflow without revert, which is not true.

asD.sol

Linked Compound docs demonstrates, that there's no need to declare redundant returnCode variable.

Cheking returnCode can be done directly in require, e.g.:

https://docs.compound.finance/v2/ctokens/#redeem-underlying

require(cToken.redeemUnderlying(50) == 0, "something went wrong");

Remove returnCode from mint(), burn(), withdrawCarry():

require(cNoteToken.mint(_amount) == 0, "Error when minting");
(...)
require(cNoteToken.redeemUnderlying(_amount) == 0, "Error when redeeming");
(...)
require( CErc20Interface(cNote).redeemUnderlying(_amount) == 0, "Error when redeeming");

Redundant variables note and exchangeRate in withdrawCarry()

There's no need to declare those variables, since they are used only once, code can be changed to:

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

SafeERC20.safeTransfer(IERC20(CErc20Interface(cNote).underlying()), msg.sender, _amount);

LinearBondingCurve.sol

Cache shareCount + amount in variable, instead of calculating it on every loop iteration in getPriceAndFee():

uint256 shareCountPlusAmount = shareCount + amount;
for (uint256 i = shareCount; i < shareCountPlusAmount; i++) {

Do-while loops use less gas than for-loop

for loop in getPriceAndFee() can be changed to do-while

Move tokenPrice variable declaration outside the loop in getPriceAndFee()

A quick test in Remix-IDE was made:

  function AAA() public view {
    uint gas = gasleft();

        for (uint i = 0; i< 100; i++){
        uint x = i;
    }
        console.log(gas - gasleft());
    }

    function BBB() public view {
        uint gas = gasleft();
        uint x;
        for (uint i = 0; i< 100; i++){
         x = i;
    }
        console.log(gas - gasleft());
    }

AAA() uses 6841 gas. BBB() uses 6451 gas. This implies, that it's better to declare variable outside the loop:

 uint256 tokenPrice;
 for (uint256 i = shareCount; i < shareCount + amount; i++) {
            tokenPrice  = priceIncrease * i;
            price += tokenPrice;
            fee += (getFee(i) * tokenPrice) / 1e18;
        }

getFee() can be rewritten

Function can return earlier, there's also no need to declare divisor variable. We can reduce the number of divisions (when divisor == 1, there's no need to perform 1e17 / divisor):

function getFee(uint256 shareCount) public pure override returns (uint256) {
    if (shareCount > 1) {
        return 1e17 / log2(shareCount);

    } else {
        return 1e17;
    }
    
    
}

Market.sol

Optimize onlyShareCreator() modifier

This modifier checks if share creation is enabled, and if not - it allows to create share only to whitelisted addresses or the owner:

whitelistedShareCreators[msg.sender] || msg.sender == owner()

Consider, in constructor(), adding owner() to whitelistedShareCreators. If owner() will be whitelisted, there'll be no need to perform that additional OR condition: || msg.sender == owner().

++shareCount; can be unchecked in createNewShare()

It's impossible that number of shares reach max uint256 value. Thus it can be unchecked.

Redundant bondigCurve variable in getBuyPrice(), getSellPrice(), getNFTMintingPrice()

bondingCurve is used only once, so we don't need to declare it at all. Code can be changed to:

(price, fee) = IBondingCurve(shareData[_id].bondingCurve).getPriceAndFee(shareData[_id].tokenCount + 1, _amount);
(...)
(price, fee) = IBondingCurve(shareData[_id].bondingCurve).getPriceAndFee(shareData[_id].tokenCount - _amount + 1, _amount);
(...)
(uint256 priceForOne, ) = IBondingCurve(shareData[_id].bondingCurve).getPriceAndFee(shareData[_id].tokenCount, 1);

claimCreatorFee() and claimPlatformFee() - perform safeTransfer() only when amount > 0.

There's missing check for amount > 0 in claimCreatorFee() and claimPlatformFee(). Whenever amount is 0, it's a waste of gas to perform safeTransfer(). It's better to verify if amount > 0 before performing transfer - the same way how it's done in claimHolderFee().

Reorganize lines in sell()

184:	tokensByAddress[_id][msg.sender] -= _amount; // Would underflow if user did not have enough tokens

Whenever amount > tokensByAddress[_id][msg.sender] above line will revert. This revert protects from selling more than msg.sender has. It would be good idea to move this line higher. Whenever sell() is called with amount greater than tokensByAddress[_id][msg.sender], function sell() will revert earlier, without wasting gas on other operations