search

vporton / courts

Crypto Reward Courts
https://reward.portonvictor.org
GNU General Public License v3.0
7 stars 2 forks source link

Do security audit of the smart contract #1

Open vporton opened 4 years ago

vporton commented 4 years ago

Do a security audit of this Solidity smart contract:

https://github.com/vporton/courts/blob/master/contracts/RewardCourts.sol

Apply only if you have security audit experience.

gitcoinbot commented 4 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 0.799 ETH (90.03 USD @ $112.68/ETH) attached to it as part of the vporton fund.

gitcoinbot commented 4 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 3 weeks, 3 days from now. Please review their action plans below:

1) ridesolo has been approved to start work.

hello, I'm an expert solidity auditor and I'm interested to audit your contract, you can check all my reports in my gist http://gist.github.com/ridesolo/. I have audited hundreds of projects the lasts two years. Also I like to take my time to do a good work so hope you are not in a hurry.

Learn more on the Gitcoin Issue Details page.

gitcoinbot commented 4 years ago

@ridesolo Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

RideSolo commented 4 years ago

Working on it.

gitcoinbot commented 4 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work for 0.799 ETH (93.56 USD @ $117.09/ETH) has been submitted by:

  1. @ridesolo

@vporton please take a look at the submitted work:

vporton commented 4 years ago

My response to your security audit: https://gist.github.com/vporton/e39a060986d02e1889aea0b50aa4356c - please respond back.

RideSolo commented 4 years ago

@vporton

i Index

For i index you are right when you say "i represent the position in the _trustees list which are courts to untrust" but the error is here:

trustedCourtsList[_truster][i] = trustedCourtsList[_truster][--max];

you are setting the last element at index i, but i does not represent the index of the court to be removed, it represent its position in _trustees array (not the position in trustedCourtsList array).

createIntercourtToken

As I commented "No owner is constrained to follow the id returned by createIntercourtToken", meaning that you didn't implement any mechanism to block owners from using the same intercourt token id. The function does return a unique id at each call, but any owner can use the same id.

Trusted courts

This is not a programing error, If you intended it to be like this then it is all right, I wanted to highlight the risks taken when implementing such logic.

Please note that as a general remark, I always try to stay neutral when I conduct an audit, I try to highlights the risks for both project team and users.

vporton commented 4 years ago

Why have you removed this?

        if (_interfaceId == INTERFACE_SIGNATURE_ERC165 ||
             _interfaceId == INTERFACE_SIGNATURE_ERC1155) {
          return true;
        }

You proposed to add checks for methods, but removed the check for these two interfaces.

I do not understand.

vporton commented 4 years ago

Could you provide the full code for setSupportedInterfaces()/supportsInterface()?

Note that the last version of the contract added two more public variables, which are also to be counted.

vporton commented 4 years ago

I do not understand why you suggest to rewrite

    function supportsInterface(bytes4 _interfaceId)
    public
    view
    returns (bool) {
        if (_interfaceId == INTERFACE_SIGNATURE_ERC165 ||
            _interfaceId == INTERFACE_SIGNATURE_ERC1155) {
          return true;
        }

        return false;
    }

in another way. You tell something about gas, but it's obvious that this function uses very little gas.

RideSolo commented 4 years ago

@vporton sure like this it is too low, however I saw that you commented out part of the value here, I set this issue to low, since If you wanted to list all the functions following ERC-165, the required gas to execute the function will be too high (probably higher than 10k or 30k suggested by the standards).

This is an audit, the recommendation done was to implement a more efficient way to list the functions, as commented " list all public function here, following the previous example" you can simply add the remaining functions by following the same example. The proposed implementation is more gas efficient since you can list as many function selectors as you want.

Also there was an error, since you set another function selector to true that must not be implemented, this is why I suggested a new way to implement it. as you can see the original code below.

    function supportsInterface(bytes4 _interfaceId)
    public
    view
    returns (bool) {
        if (_interfaceId == INTERFACE_SIGNATURE_ERC165 ||
            _interfaceId == INTERFACE_SIGNATURE_ERC1155 ||
            _interfaceId == INTERFACE_SIGNATURE_URI) {
          return true;
        }

        return false;
    }

Please note that it was just a suggestion and you can decide to ignore it. the main point was to point out the issue in case of setting more function selectors.

vporton commented 4 years ago

Why should I add "all functions"?

INTERFACE_SIGNATURE_ERC165 and INTERFACE_SIGNATURE_ERC1155 are already checked. I am confused.

RideSolo commented 4 years ago

I agree, I pointed this issue out because I saw you comments here I figured out that you wanted to implement all functions and using condition while accessing all global variables won't be optimal. also you should note that it is a low severity issue and it does not represent any risk .

gitcoinbot commented 4 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

The funding of 0.799 ETH (93.56 USD @ $117.09/ETH) attached to this issue has been approved & issued to @RideSolo.