Kolor Audit Report

Mappings in safeMint() function don’t get updated with the correct address. This causes several critical functions to not work properly. There are two important address type variables passed into function KolorLandNFT.safeMint(), which are to and landOwner. All land NFTs are minted to the to address, which is an address controlled by the Kolor team. The function only records and updates the NFT owner (i.e., landOwner) in the internal mappings mintedNFTSInfo, ownedLand, _totalLandOwned. However, the address to is used when updating some of those mappings where landOwner should be used. Setting this incorrectly causes ownedLands and _totalLandsOwned to update the wrong address. This affects functions like totalLandOwnedOf(), landOfOwnerByIndex(), updateLandOwner(), etc.

1. Kolor team mints NFT and set Alice as the landOwner;

2. Since mapping are not correctly updated, landOwner mappings get assigned to Kolor team;

3. Several functions cannot work correctly because of the wrong information in the mapping.

Update mappings using correct addresses. Consider the following lines:

// set the tokenId to current landowner collection index
ownedLands[to][_landsOwned] = currentTokenId;

// increase total lands owned by address
_totalLandOwned[to]++;

Change them to this:

// set the tokenId to current landowner collection index
ownedLands[landOwner][_landsOwned] = currentTokenId;

// increase total lands owned by address
_totalLandOwned[landOwner]++;

Resolved in commit 7c091960656518c20169e3493ec476f44938e93b.

landOwner now has been used to update the mappings.

Functions removeLand() and updateLandState() remove NFTs from the marketplace by changing the published state. However, there’s no logic to returning the emissions offset to buyers or tokens to investors and returning NFT to the sender. These functions update the state of the land token. So, it does not allow any more buyers to offset emissions on the land NFTs. Nevertheless, if a buyer had already offset emissions, they would lose all the TCO2 since there’s no logic to return the already offset emissions. Additionally, there’s no restriction for investors to add investments but they should get their tokens back when the state of the NFT is changed. Buyers and investors cannot recover their emissions and tokens respectively when the state is switched, which prevents them from using those emissions and tokens on other NFTs.

Buyer offsets emissions on land NFT that is already in the marketplace and the user already has investment on it, and then the NFT is removed from it by updating its state. Now, the buyer is not able to recover those emissions and the user is not able to recover tokens unless the state is once again changed to published.

Add logic to return the emissions to buyers and tokens to investors when the land NFT is removed from the marketplace and returned to the initial sender.

Resolved in commit 4ce7e209dcdb3842b545aa14585359a727479abf.

Land NFT is now transferred back to the owner when calling the removeLand() function.

The require statement in function landOfOwnerByIndex() uses the balanceOf() the landOwner instead of totalLandOwnedOf(). The false require statement in landOfOwnerByIndex can cause unexpected reverts. Function balanceOf() is inherited from ERC721.sol and therefore returns the count of NFT minted by a certain address. Nevertheless, when creating an NFT one can assign a different landOwner, sobalanceOf() would not be really counting the number of lands owned by a certain address which is what function landOfOwnerByIndex() tries to return for an input index. Not checking with the correct function in the require statement of function landOfOwnerByIndex() can cause a revert.

Kolor team mints an NFT and updates the landOwner to Alice, then Alice is the owner of one NFT but calling landOfOwnerByIndex() will cause a revert since the NFT wasn’t minted to her.

Change balanceOf() to totalLandOwnedOf() in the require statement in function landOfOwnerByIndex(). Also update _totalLandOwned mapping when calling updateLandOwner().

Resolved in commit 4ce7e209dcdb3842b545aa14585359a727479abf.

Mapping _totalLandOwned now gets updated when calling updateLandOwner(). Additionally, require statement for function landOfOwnerByIndex() has been fixed.

The protocol has access control risks. Since the project team address has the ability to control almost all the functionality, a key leakage would have devastating effects. Most of the functions use the onlyAuthorized() modifier which means only the account address authorized can get access to the functionality. Since this is set to true for the sender in the constructor(), the authorization relies solely on one account in the beginning. A private key leakage from the authorized address will result in the loss of both control and access to the protocol.

If the team holds the key and the key got leaked, an attacker can transfer ownership of the contracts, authorize their address and unauthorize the team’s address. This will lead to the team not having access and control over the protocol anymore.

Implement an extra admin layer to the protocol following something similar to a hot and cold wallet admin system.

• Access control

  Most functions for the three contracts come with the onlyAuthorized() modifier that only allows authorized addresses to execute functions. This is set to true on the deployment of the contract for the sender of the transaction, however, other addresses can get authorized by calling the authorize() function on any of the three contracts. The authorize() function can only be called by the owner of the respective contract.

  Since the access control for the protocol relies very heavily on only one address, we recommend the use of an extra admin layer, so that in case the private key of the admin address gets leaked, the funds will not get drained. In this case, a cold wallet will be the admin storing the protected keys and will allow the hot wallet to perform all the functionality.

• OpSec on the usage of hot and cold wallet

  The two-layer admin system would consist of a cold wallet, of which keys will be used really infrequently, preferably only in the case of an attack, and a hot wallet that will be the one that performs all the logic of the protocol and which keys will be used very often. The cold wallet will be able to set and revoke the hot wallet in charge of executing the functionality of the project.

  The use of this system allows for extra security throughout the entire protocol since the hot wallet is making many transactions, and it becomes really exposed to possible attacks. Therefore, having this extra layer prevents from losing control over the protocol if the keys of the hot wallet get compromised or hacked.

Resolved in commit 4ce7e209dcdb3842b545aa14585359a727479abf.

owner will authorize an account to perform all the functionality of the protocol. If the key gets leaked, owner will unauthorize that account and authorize a different account.

Function offsetEmissions() is payable, therefore the contract is able to receive CELO through that function and there’s no way to withdraw it. The payable functions in solidity are able to receive the native currency of the blockchain when being called. However, for this contract, this is not necessary since all the functionality lies in updating variables and mappings. Also, since there’s no withdraw function in the contract, there’s no way to recover sent tokens through that function. So, mistakenly sending tokens through this function will get them locked in the contract forever.

If mistakenly send CELO through that function, those tokens will be locked forever.

Remove the payable keyword and the getBalance() function.

Resolved in commit 4ce7e209dcdb3842b545aa14585359a727479abf.

The payable keyword has been removed.

KolorLandToken.setLandTokenInfo() initializes info for land tokens. However, this function does not check whether the land token has been set before and thus the land token info can be initialized again. Function KolorLandToken.setLandTokenInfo() sets the land token’s initial amount, sold amount, and creation date. By setting the initial amount, the amount will be added to the land token’s current amount and available amount. And the sold amount will be reset to 0. However, these are critical info that will be updated by other functions like KolorLandToken.NewInvestment(). By being able to set the land token info again, that critical Investment info for this land token can be reset or overwritten.

1. Authorized account Alice set initial land info for land token 123 by calling function KolorLandToken.setLandTokenInfo();

2. Alice adds a new investment for investor Bob on land token 123 by calling the function KolorLandToken.NewInvestment();

3. Alice called the function KolorLandToken.setLandTokenInfo() to reset the investment amount and sold amount on the land token.

Check if the land token info initialized in KolorLandToken.setLandTokenInfo().

function setLandTokenInfo(uint256 tokenId, uint256 initialAmount) external onlyAuthorized {
    require(exists(tokenId), "KolorLandToken: land must exists!");
		require(landTokensInfo[tokenId].initialAmount == 0, "KolorLandToken: land token has initialized!");

    landTokensInfo[tokenId].initialAmount = initialAmount;
    landTokensInfo[tokenId].sold = 0;
    landTokensInfo[tokenId].creationDate = block.timestamp;

    addNewTokens(tokenId, initialAmount);
}

Resolved in commit 4ce7e209dcdb3842b545aa14585359a727479abf.

Require statement has been added to prevent this.

Function KolorLandToken.addNewTokens() calls function KolorLandToken.mint() to mint new tokens. The function call may fail because KolorLandToken.mint() function requires only an authorized caller. When function KolorLandToken.addNewTokens() calls the function KolorLandToken.mint(), the caller of function KolorLandToken.mint() is the contract itself, address(this). Function KolorLandToken.mint() requires the caller to be an authorized user only. The function call may revert if the contract itself is not added to the authorized address list.

Contract KolorLandToken.sol hasn’t been authorized and authorized user calls addNewTokens(), transaction will revert because the internal call is done from KolorLandToken.sol which isn’t authorized.

In the contract design, we recommend marking the function KolorLandToken.mint() be private or internal if it will only be called inside function KolorLandToken.addNewTokens().

Acknowledged.

Kolor team will call the authorize() function on the contract to authorize the contract address.

The default value 0 is the return result when accessing mapping KolorLandNFT.ownedLands. Its default return is 0 when the mapping is not set. To avoid confusion between token id 0 and empty value, the land NFT token id should increment from a non-zero value. The mapping KolorLandNFT.ownedLands returns the land NFT token id given owner and the owner’s land NFT index. As the mapping’s default return is zero when a value or mapping does not exist, we might confuse it with the return of the actual NFT token id.

1. Alice has an NFT, token id is 0, index 7;

2. Bob does not own any land NFT;

3. The mapping KolorLandNFT.ownedLands return 0 when querying with Bob’s address;

4. The system will assume Bob owned the token id 0 NFT.

Start Land NFT’s token id from 0.

Acknowledged.

Kolor team decided to leave this part as it was.

KolorLandNFT.ownedLands is not updated when updating land owners.

Function KolorLandNFT.updateLandOwner() is used to update a land’s owner. Mapping KolorLandNFT.ownedLands which records all lands owned by a person is not updated when the function updates the land owner. The mapping KolorLandNFT.ownedLands is also used in function KolorLandNFT.landOfOwnerByIndex(). Given the land owner and the index of the land owner’s land, it will return the corresponding land token Index. If the mapping KolorLandNFT.ownedLands is not updated correctly, then false results might be returned when we query the function. Land may have been transferred to others but the function still returns the land’s previous owner.

1. Alice has one land NFT with token id 678 and the index is 100 in mapping ownedLands;

2. The land owner has been updated to Bob;

3. When searching the land index in Alice’s owned lands, it still returns land NFT 678.

Update ownedLands mapping when calling updateLandOwner() function.

When updating the land owner with the given token id and new land owner:

1. Update the mintedNFTInfo and Iterate through the current land owner’s lands to get the correct land id IdOfLandNeedsUpdated;

2. Update the old owner’s ownedLands:
   1. Get the last token index of the current owner by using _totalLandOwned[oldOwner] and then get the token id with the index:

      uint256 lastTokenIndex = _totalLandOwned[currentOwner] - 1; // need to check 
      uint256 tokenId = ownedLands[oldOwner][lastTokenIndex];

   2. Move the last land NFT token to the position where the land needs to be removed and update the _totalLandOwned[currentOwner]:

      ownedLands[currentOwner][IdOfLandNeedsRemove] = tokenId;
      ownedLands[currentOwner][lastTokenIndex] = 0; // assume 0 represent empty / not exist. see the finding Medium 7
      _totalLandOwned[currentOwner]--;

3. Update the new owner’s ownedLands:
   1. Append the land NFT to the new owner and update the _totalLandOwned[oldOwner]:

      uint256 newTokenIndexOfNewOwner = _totalLandOwned[newOwner];
      ownedLands[newOwner][newTokenIndexOfNewOwner] = IdOfLandNeedsUpdated;
      _totalLandOwned[newOwner]++;

Resolved in commit 7c091960656518c20169e3493ec476f44938e93b.

The updateLandOwner() function was removed. Kolor team acknowledges that this results in not being able to change the land owner after minting the NFT anymore.

Function addBuyer() doesn’t check whether the buyer has already been added or not. The missing check for this might cause totalBuyers[tokenId] to be bigger than it should be as it is incremented even if a buyer already exists:

function addBuyer(uint256 tokenId, address newBuyer) public override onlyMarketplace notBurned(tokenId){
    buyers[tokenId][newBuyer] = true;
    totalBuyers[tokenId]++;		
}

Function addBuyer() adds input address to mapping totalBuyers that counts the number of addresses that have offset emissions on certain land. However, this function doesn’t check whether the input address has already been added or not. This can result in counting the same address multiple times.

1. Kolor team adds a buyer to offset emission on a land NFT;

2. Kolor team adds the same buyer to offset emissions on that same NFT;

3. Mapping totalBuyers buyer is 2 instead of 1.

Change it to this:

function addBuyer(uint256 tokenId, address newBuyer) public override onlyMarketplace notBurned(tokenId){
    if(!buyers[tokenId][newBuyer]) {
		    buyers[tokenId][newBuyer] = true;
        totalBuyers[tokenId]++;
    }
}

Resolved in commit 7c091960656518c20169e3493ec476f44938e93b.

Fixed as suggested.

Function safetransferFrom() doesn’t update mapping variables totalInvestmentsByAddress, totalInvestmentsByLand, and totalInvestments when called directly by users.

1. Alice gets investment for a land NFT;

2. Alice calls the safeTransferFrom() to send part of the tokens that represent the investment to Bob;

3. Since mappings weren’t updated, functions that depend on those mappings will not consider the new investment. For example, calling totalInvestmentsOfLand() will not consider Bob’s investment.

Update mapping variables in safetransferFrom() function.

Acknowledged.

Kolor team expects users not to call the safeTransferFrom(). If they call it, they will consider only the initial investment. No view function for mapping KolorLandNFT.landIndex.

Mapping landIndex gets updated only at KolorLandNFT.safeMint() and it doesn’t have public getter functions. This makes it difficult to get values directly from it.

N/A.

Change the mapping’s visibility from private to public or create a public getter function for the mapping.

Resolved in commit 7c091960656518c20169e3493ec476f44938e93b.

A getter function was added.

Function onERC721Received() visibility can be restricted to pure since it is only returning the selector of the function. This allows for gas optimization.

N/A.

Change visibility of the function to pure.

Acknowledged.

Internal function isLandOwner() in KolorLandNFT.sol is never used. Private mapping lockStartTime is defined but never used.

N/A.

Remove that function or change visibility.

Resolved in commit 4ce7e209dcdb3842b545aa14585359a727479abf.

There are functions with the same but with different parameters. It’s good practice to have explicit and explanatory names for each function. Additionally, having the same name for two functions that have the same number of parameters can be confusing to other developers.

N/A.

Change the names of functions so that names are different.

Resolved in commit 4ce7e209dcdb3842b545aa14585359a727479abf.

There are no comments in some functions. The use of natspec is recommended for writing comments and it is important for crucial functions so that other developers can easily understand the use of these functions and how to interact with them.

N/A.

Add comments to the functions specified in the source.

Acknowledged.

Input parameters address operator, address from, uint256 tokenId and bytes calldata data are not used on function onERC721Received().

N/A.

Comment unused parameters.

Acknowledged.

Typo in comment for require statements and in developer comment.

require(erc721.ownerOf(tokenId) != address(0), "Token doesn't exists!");

/** @dev Adds a new offset structure to a account to represent

N/A.

Change the word “exists” to “exist” and “a account” to “an account”.

Acknowledged.

Current smart contracts use ^0.8.0. And compilers within versions≥ 0.8.0 and <0.9.0 can be used to compile those contracts. Therefore, the contract may be deployed with a newer or latest compiler version which generally has higher risks of undiscovered bugs. It is a good practice to fix the solidity pragma version if the contract is not designed as a package or library that will be used by other projects or developers

N/A.

Use the fixed solidity pragma version.

Acknowledged.

Update the require condition to receive proper revert messages. In the following require statement, the transaction will not revert with the desired message “KolorLandToken: exceeds max amount” when availableTokensOf(tokenId) is smaller than the amount. It will only revert with the subtraction underflow error message.

require(availableTokensOf(tokenId) - amount >= 0,
    "KolorLandToken: exceeds max amount"
);

N/A.

Change the require statement to:

require(availableTokensOf(tokenId) >= amount,
    "KolorLandToken: exceeds max amount"
);

Resolved in commit 4ce7e209dcdb3842b545aa14585359a727479abf.

Check check-effects-interactions pattern for functions like function with external function calls, such as KolorLandToken.newInvestment() to reduce the attacking surface. Check states first, then apply changes to related states and call the external function in the final step. In this way, attacking surfaces for the external function is reduced. For normal external functions without before-call hooks, there is no surface for them to reenter the function because there is no attacking interface after the external function call.

N/A.

Follow check-effects-interactions practices.

Acknowledged.

Function comment says that an event is emitted when calling setApprovalForAll(), but there are no events emitted in the contract and specifically in this function.

* @dev Approve `operator` to operate on all of `owner` tokens
*
* Emits a {ApprovalForAll} event.
*/
function setApprovalForAll(
    address owner,
    address operator,
    bool approved
) internal virtual {
    require(owner != operator, "ERC1155: setting approval status for self");
    operatorApprovals[owner][operator] = approved;
    //emit ApprovalForAll(owner, operator, approved);
}

N/A.

Event emission is recommended as it can help monitor the function related to critical operations.

Acknowledged.

Contract KolorLandNFT.sol, interface IKolorLandNFT, and IERC721 are used to make calls at the same time. This is unnecessary since the contract has already all the functionality and all the calls could be made by directly importing it. Even better would be if the interface had access to all the functionality and this was the one getting used to making the calls.

IKolorLandNFT ilandInterface = IKolorLandNFT(KolorLandNFTAddress);
KolorLandNFT kolorLand = KolorLandNFT(KolorLandNFTAddress);
IERC721 erc721 = IERC721(KolorLandNFTAddress);

N/A.

Make the interface IKolorLandNFT inherits IERC721 and add other needed functions to interface IKolorLandNFT. In this way, only IKolorLandNFT is needed. No need to use all three of them.

Acknowledged.