This document is a security audit report performed by ridesolo, where Court Reward has been reviewed.
- RewardCourts.sol github commit hash 7115b6f45ed5ba923ce4fea3f26a115b29c946df.
- Common.sol github commit hash 8ebabf945595890c3f2f77d6c5b320aff17111fc.
- Address.sol github commit hash 8ebabf945595890c3f2f77d6c5b320aff17111fc.
- ERC165.sol github commit hash 8ebabf945595890c3f2f77d6c5b320aff17111fc.
- IERC1155.sol github commit hash 8ebabf945595890c3f2f77d6c5b320aff17111fc.
- IERC1155TokenReceiver.sol github commit hash 8ebabf945595890c3f2f77d6c5b320aff17111fc.
** 8 ** were reported including:
- 1 high severity issue.
- 3 medium severity issues.
- 2 low severity issues.
- 2 notes.
A chain of trust is implemented when owners trust each other courts, however this logic represent a lot of risks since if a single malicious owner (or a private key hack) access to any chain of trust he will be able to mint internal tokens in other courts.
This logic will allow different kind of attacks (especially if the tokens have a significant price or usage).
The developers used trustedCourtsList to save and get the trusted court ids through an array, when untrusting a court the id is correctly set to false inside the mapping but when removing the id from the array, id i is replaced by the last element of the array which is wrong. i does not represent the index position of the court id to untrust, it represent the position of the id in the _trustees input court list.
Another note is that trust function uses two for loops instead of one. When a court owner adds trusted courts using trustCourts function instead of checking the existance of _trustees list elements in trustedCourtsList array and compute a new length, the developers can directly push the new court ids to the array in the same loop without requiring .
To solve this issue please follow the recommendation below.
Please note that this is just recommendation and should be cross checked by the developers before implementation.
mapping (uint128 => mapping (uint128 => uint256)) internal trustedCourtsIndex;
mapping (uint128 => mapping (uint128 => bool)) internal trustedCourtstrustedCourtsIndex;
mapping (uint128 => uint128[]) public trustedCourtsList;
function trustCourts(uint128 _truster, uint128[] _trustees) external {
require(courtOwners[_truster] == msg.sender, "We are not court owner");
for (uint i = 0; i < _trustees.length; ++i) {
require(_trustees[i] > 0 && _trustees[i] <= courtNonce, "Court does not exist.");
if(!trustedCourts[_truster][_trustees[i]]) {
trustedCourts[_truster][_trustees[i]] = true;
trustedCourtsIndex[_truster][_trustees[i]] = trustedCourtsList[_truster].length;
trustedCourtsList[_truster].push(_trustees[i]);
}
}
emit TrustCourts(_truster, _trustees);
}
function untrustCourts(uint128 _truster, uint128[] _trustees) external {
require(courtOwners[_truster] == msg.sender, "We are not court owner");
uint index;
uint length = trustedCourtsList[_truster].length;
for (uint i = 0; i < _trustees.length; ++i) {
require(_trustees[i] > 0 && _trustees[i] <= courtNonce, "Court does not exist.");
if (trustedCourts[_truster][_trustees[i]]) {
trustedCourts[_truster][_trustees[i]] = false;
index = trustedCourtsIndex[_truster][_trustees[i]];
trustedCourtsList[_truster][index] = trustedCourtsList[_truster][--length];
delete trustedCourtsIndex[_truster][_trustees[i]];
}
}
trustedCourtsList[_truster].length = length;
emit UntrustCourts(_truster, _trustees);
}
_doIntercourtTransferBatch function does transfers between courts with a conversion ratio of 1:1. For example if tokens in court x have a higher price than tokens in a court y, users will be able to transfer their tokens toward that court if the chain of trust is established by the owners.
This issue goes against what is described in intercourtTransfer and intercourtTransferBatch "Transfer money through several courts with automatic currency conversion between court tokens", since no conversion mechanism is applied.
Following ERC-1155 standard "If the optional extension is included: The ERC-165 supportsInterface function MUST return the constant value true if 0x0e89341c is passed through the interfaceID argument". This impelemntation does not contain the ERC1155Metadata_URI extension, meaning that the developers must set 0x0e89341c when passed through supportsInterface to false or implement the extension.
When setting the new owner address for a court:
- The new owner address should be verified to be different than zero address.
- Two step approvals must be set to avoid setting a wrong addresses. A first function will save an address in a global variable, the second function will confirm the new address if the msg.sender is equal to the new address proving that the new owner has access to the correct private key.
EIP-165 intend to "creates a standard method to publish and detect what interfaces a smart contract implements", however this implemented version standard does not set all the functions except the required ERC-1155 functions and ERC-165, a mapping can be used to set all functions and avoid consuming more gas than the intended 10K of EIP-1155 (the requirement for EIP-165 is 30K). Please refer to the recommendation.
mapping(bytes4 => bool) internal supportedInterfaces;
function setSupportedInterfaces() internal {
// example
supportedInterfaces[bytes4(keccak256('supportsInterface(bytes4)'))] = true;
// list all public function here, following the previous example.
}
function supportsInterface(bytes4 interfaceID) external view returns (bool) {
return supportedInterfaces[interfaceID];
}
constructor() {
setSupportedInterfaces();
}
No owner is constrained to follow the id returned by createIntercourtToken, this function can simply be removed if the developers do not intend to use it.
Using "inter" as a prefix is confusing since it can means both "between" or "within", a clear distinction should be done when using it in function or variables.
All highlighted issues should be solved before deployment for users and team benefits.