ailisp commented 4 years ago

2. ProofDecoder.sol (6993b73)

In this section we describe issues related to the smart contract defined in the ProofDecoder.sol .

2.1 Moderate Flaws

This section lists moderate flaws, which were found in the smart contract.

[x] Line 128-131: the code may loose information in case if len is >= 2^32.

Bo: This is impossible, given:

        outcome.merkelization_hashes = new bytes32[](1 + outcome.logs.length);

And logs.length is not possible to exceed 2^32-1 in nearcore given the gas limit, am i correct? @nearmax @bowenwang1996

2.2 Suboptimal Code

This section lists suboptimal code patterns, which were found in the smart contract.

[ ] 1. Line 49-51: the fields structure is packed compactly into 32-byte words, but never crosses word boundary. Actually, 29 padding bytes will be added after these fields. Consider reordering fields to avoid padding.
[ ] 2. Line 60, 65: next assignments are redundant: ● executionStatus.unknown=trueasenumIndex==0 implies unknown == true. ● executionStatus.failed=trueasenumIndex==1implies failed == true.
[ ] 3. Line 62: the if statement is indented, but is independent from the previous one. Actually it is inside the else branch of the previous if statement. Consider changing indentation.
[ ] 4. Line 85: all but the first element ExecutionOutcome struct seem to be just hash of logs. So it does not make much sense to store and pass those together with logs. Probably one of them can be dropped.
[ ] 5. Line 140: it would be more efficient to store all directions as a bit mask in a single word. This word could be treated as a position of a leaf in a Merkle tree.

2.3 Other Issues

This section lists other minor issues which were found in the token smart contract.
[ ] 1. Line 59, 62, 6 7, 7 0: assignment values 0, 1, 2, 3 should be named constants.
[ ] 2. Line 106-108: the code would be more readable, less error-prone, and more efficient in case if the peekSha256 would accept to parameters: offset and length, instead of just length.
[ ] 3. Line 123: using the same name as the outcome.outcome for different things inside one sentence is confusing. Consider using different naming.

3. NearProver.sol (3563521)

In this section we describe issues related to the smart contract defined in the NearProver.sol.

3.1 Suboptimal Code

This section lists suboptimal code patterns, which were found in the smart contract.

[ ] 1. Line 24: the proveOutcome function always returns true. Consider removing returns clause.
[ ] 2. Line 61: the directions could be encoded as a bit mask. Such bit mask could be treated as a position of a leaf inside Merkle tree and could be passed as a single word. This would make code more efficient.
3.2 Other Issues

This section lists other minor issues which were found in the token smart contract.
[ ] Line 4: the next files are imported but not used: ● SafeMath.sol ● Ownable.sol ● NearDecoder.sol
4. NearBridgeMock.sol ( b92a80f)

In this section we describe issues related to the smart contract defined in the NearBridgeMock.sol .

4.1 Suboptimal Code

This section lists suboptimal code patterns, which were found in te smart contract.

[ ] Line 10-15: the functions setBlockMerkleRoot and setBlockHash could be combined into one that sets Merkle root and hash for a block at once.
5. NearDecoder.sol (4229cd7)

In this section we describe issues related to the smart contract defined in the NearDecoder.sol .

5.1 Suboptimal Code

This section lists suboptimal code patterns, which were found in the smart contract.
[ ] 1. Line 7: the library code could be made much simpler and more efficient if encoded format of various data structured would be compatible with Solidity's the abi.decode function.
[ ] 2. Line 15-16: while for each particular instance of PublicKey structure only one of these fields is used, both fields are being stored and transferred, which is suboptimal. Consider reusing fields.
[ ] 3. Line 34: strings are expensive in Solidity. Consider using hash of the account ID instead.
[ ] 4. Line 53, 96: according to the code ( data.decodeU8() == 0) any non-zero value is treated as true. This is error-prone. Consider enforcing only one valid true value, for example 1 or 0xFF.
[ ] 5. Line 63-65: it would be much simpler, less error-prone, and more efficient, if the function peekSha256 would accept two parameters: the offset and the length, instead of just the length.
[ ] 6. Line 141, 146: the structure fields are packed compactly into 32-byte words, but never cross work boundary. So 24 padding bytes will be added after this fields. Consider reordering structure fields to improve efficiency.
5.2 Other Issues

This section lists other minor issues which were found in the token smart contract.
[ ] 1. Line 3: the file SafeMath.sol imported but not used.
[ ] 2. Line 22: the assignment values 0, 1, should be named constants.
[ ] 3. Line 110: the hash name is confusing. There are many hashes in the block, and it is unclear what this hash is for. Consider renaming.

6. Borsh.sol (6993b73)

In this section we describe issues related to the smart contract defined in the Borsh.sol .

6.1 Critical Flaws

This section lists critical flaws, which were found in the smart contract.

[x] Line 70: the int16 c onversion extends sign bit, so in case the first call to decodeI8(data) returned negative value, the high 8 bits of the result will be filled with ones regardless of what the second call to decodeI8(data) will return.

6.2 Arithmetic Overflow Issues

This section lists issues of the smart contract related to the arithmetic overflows.
[x] Line 23, 39: the overflow possible in these operations: ● data.offset+size ● add,add

6.3 Suboptimal Code

This section lists suboptimal code patterns, which were found in the smart contract.
[ ] 1. Line 32, 36, 4 3, 4 7: there is no range check for: ● the length parameter. In case length > data.raw.length - data.offset it will read memory after the data. ● the offset parameter. In case offset + length > ptr.length it will read memory after the data.
[ ] 2. Line 39, 51: the memory range may wrap in case l ength > 2^256 - add(add(ptr, 32), offset).
[ ] 3. Line 51: the pop operation ignores execution status of SHA256 precompiled smart contract. While this contract shouldn't fail in current implementation, it is still a bad practice to ignore such things.
[ ] 4. Line 56: the shift modifier changes the data. The function should not be declared as pure.
[ ] 5. Line 66: the calling decodeU8 function twice inside one function is suboptimal, as range check is performed twice, and offset is also updated twice. Consider implementing decodeU16 function without references to decodeU8.
[ ] 6. Line 104: the decodeU256 function is suboptimal as well as other similar functions, as it reverses byte ordering one by one and performs many internal function calls. Consider reading 32 bytes with a single mload, and then reverting byte ordering in 8 rounds similar to how this is done in Ed25519.sol.
[ ] 7. Line 115: the decodeU8(data) != 0 allows many different encodings for true value. Enforcing only one valid encoding for true would make code less error prone.
[ ] 8. Line 120: the copying byte one by one is suboptimal. Consider copying in 32-byte words when possible.
[ ] 9. Line 135: the 20 byte could be read using single mload opcode. Reading them one by one is suboptimal.
[ ] 10. Line 136: the bytes20 will internally shift value left, just to then shift it right.

6.4 Unclear Behaviour

This section lists issues of the smart contract, where the contract behavior is unclear: the business logic might be violated here, but the documentation and functional requirements are not sufficiently documented to make a clear decision.
[ ] Line 147: the decodeSECP256K1PublicKey function doesn't check that decoded coordinates fit into field order, and doesn't check that decoded point actually lays on the curve. Is it OK or not?.

6.5 Other Issues

This section lists other minor issues which were found in the token smart contract.
[ ] 1. Line 3: the SafeMath.sol file imported but not used.
[ ] 2. Line 6: the library name Borsh is confusing. Consider using more descriptive name.
[ ] 3. Line 22: the modifier shift does not shift the data, but rather consumes certain number of first bytes. Consider renaming to consumes.
[ ] 4. Line 64: the documentation comment for the decodeU16 function needed. It should be said that the function does the integer reading in little endian.
[ ] 5. Line 136: the operation & 0xFF seems redundant.

7. NearBridge.sol (4031aa8)

In this section we describe issues related to the smart contract defined in the NearBridge.sol.

7.1 Moderate Flaws

This section lists moderate flaws, which were found in the smart contract.

[x] 1. Line 37: most of the public functions in this contract don't check that the contract is initialized, even those functions that read values that ought to be set during initialization. This looks dangerous. Consider adding explicit checks to all the functions that are not supposed to be called before initialization.
[x] 2. Line 121: the initWithBlock function can be called by anyone. This could allow an attacker to steal smart contract after deployment but before initialization.

7.4 Suboptimal Code

This section lists suboptimal code patterns, which were found in the smart contract.

[ ] 1. Line 21: the size of some fields in the State struct could be reduced to save storage space and thus gas. For example, validAfter could be reduced to 32 or 40 bits; approvals_after_next_length and next_bps_length could also be reduced to 8 or 16 bites; probably next_total_stake could probably be reduced to 128 bits.
[ ] 2. Line 23: the structure fields are packed into 32-byte words compactly, but never crosses work boundary. The 24 padding bytes will be inserted before this field. Consider reordering the fields to avoid padding. For example, use the following ordering: bytes32 epochId; bytes32 nextEpochId; uint256 validAfter; bytes32 hash; bytes32 next_hash; uint256 approvals_after_next_length; uint256 next_bps_length; uint256 next_total_stake; uint64 height; address submitter; mapping(uint256 => NearDecoder.OptionalSignature) approvals_after_next; mapping(uint256 => BlockProducer) next_bps;
[ ] 3. Line 29-30: the d ynamic array would be more convenient and probably more efficient alternative to a pair of a mapping and a counter.
[ ] 4. Line 34: the dynamic array would be more convenient and probably more efficient alternative to a pair of a mapping and a counter.
[ ] 5. Line 58: the Ed25519 ed parameter should be a library linked at compile, rather than deployment, time. Also, there is no range check for the l_eth parameter. Is zero a legal value for it? If no, consider adding explicit check.
[ ] 6. Line 66: the balanceOf[msg.sender].add(msg.value) expression could be replaced with lock_eth_amount to save gas. Also, since a deposit can be made only once and the amount is fixed, a boolean mapping empty/nonempty would suffice.
[ ] 7. Line 71: the balanceOf[msg.sender].sub(lock_eth_amount) expression is always zero when evaluated successfully. Consider simplifying.
[ ] 8. Line 76: withdraw is possible when block.timestamp > last.validAfter, challenge is possible when block.timestamp < last.validAfter, and adding light client block is possible when block.timestamp >= last.validAfter, so situation when block.timestamp == last.validAfter differs from both block.timestamp > last.validAfter and block.timestamp < last.validAfter. Consider simplifying.
- [ ] 9. Line 100: in the sub operation important business-logic check is delegated to the utility function SafeMath.sub. This makes code less reliable and harder to read. Consider writing all business-logic checks explicitly.
[ ] 10. Line 101: in this line another address is called before updating the storage. While with transfer it is quite safe, best practice is to call external untrusted addresses after all the state changes.
[ ] 11. Line 112-118: copying complex structures could be very expensive in Ethereum and may even exceed block gas limit. Consider implementing a backup strategy that doesn't require copying.
[ ] 12. Line 189: the variable-length loops (as next_bps_length) can exceed gas limit. Consider explicit limiting the upper bound of such loops.
[ ] 13. Line 237: the blockHashes[last.height] value was just written to the storage. Consider caching in local variable and reusing to save gas.
[ ] 14. Line 250:the values bytes32 arg1, bytes9 arg2 could be calculated from next_block_hash and _reversedUint64(height + 2) by using shifts and bitwise operations. No need to use ABI encode/decode.
[ ] 15. Line 265-268: it would be cheaper to derive address from public key off-chain, and pass only the derived address to the smart contract. It doesn't seems that the smart contract uses public key for any purpose other than deriving address from it.
[ ] 16. Line 278:this looks like a plain assignment, while actually this returns value from the function. Consider refactoring.

7.5 Unclear Behaviour

This section lists issues of the smart contract, where the contract behavior is unclear: the business logic might be violated here, but the documentation and functional requirements are not sufficiently documented to make a clear decision.

[ ] 1. Line 173: perhaps, the comment is incorrect. Actually it is less than or equal.
[ ] 2. Line 232, 233: there is no check for the height value. Is it ok?
7.6 Other Issues

This section lists other minor issues which were found in the token smart contract.
[ ] 1. Line 28: while some other fields in this structure are named in camelCase, this field name uses words_separated_by_underscore. Consider using consistent naming.
[ ] 2. Line 48, 53: the event BlockHashAdded and the BlockHashReverted is already declared in INearBridge. There is no need to redefine it.
[ ] 3. Line 272: the _reversedUint64 function should be moved to a library of utility functions.
[ ] 4. Line 274: these operators are redundant: ● & 0x00000000FFFFFFFF ● & 0xFFFFFFFF00000000

8. INearBridge.sol (1b1f5fd)

In this section we describe issues related to the smart contract defined in the INearBridge.sol (1b1f5fd).

8.1 Documentation Issues

This section lists documentation issues, which were found in the smart contract

[ ] 1. Line 4: the functions semantic INearBridge interface is unclear from their names and signatures. Consider adding documentation comments.
[ ] 2. Line 20: It is unclear how much ether the function withdraws. Consider adding documentation comment. Also, the function should probably return the amount with ether withdrawn.
8.2 Other Issues

This section lists other minor issues which were found in the token smart contract.
[ ] 1. Line 6: if the height the same as the b lock number consider using consistent terminology.
[ ] 2. Line 15,16: the blockHashes function seems to return only one block hash, but the name is plural. Consider renaming to the blockHash or the blockHashByNumber or something like this. The same comment for the blockMerkleRoots.

Summary

Based on our findings, we also recommend the following:

[x] 1. Fix critical issues which could lead to incorrect checks.
[x] 2. Pay attention to moderate issues.
[x] 3. Fix arithmetic overflow issues.
[ ] 4. Check issues marked “unclear behavior” against functional requirements.
[ ] 5. Refactor the code to remove suboptimal parts.
[ ] 6. Fix the documentation, readability and other (minor) issues.

MaksymZavershynskyi commented 4 years ago

I am going to close this issue to not confuse people. @ailisp , @Kouprin let's go over the audits and create issues for each of the findings that were not addressed yet. @ailisp , thank you for drafting the document!

ailisp commented 4 years ago

@Kouprin I found i only created issues for the concensys report, the solidity report and today's fuzz test report is missing, I think they maybe more suitable to create as one big issue than a few smaller issues.

MaksymZavershynskyi commented 4 years ago

@ailisp Thank you for converting this into a checkpoint list.

Kouprin commented 4 years ago

According @nearmax idea about having the only assigned person, I withdraw my assignment.

MaksymZavershynskyi commented 4 years ago

Might require quite some time to address and test all of them. Setting estimate to 3.

MaksymZavershynskyi commented 4 years ago

For the record, as discussed @ailisp will address Major and Critical flaws (except ED25519) and @abacabadabacaba will take over the rest.

near / rainbow-bridge-sol

Address solidity contract audit issues #12

2. ProofDecoder.sol (6993b73)

2.1 Moderate Flaws

2.2 Suboptimal Code

2.3 Other Issues

3. NearProver.sol (3563521)

3.1 Suboptimal Code

3.2 Other Issues

4. NearBridgeMock.sol ( b92a80f)

4.1 Suboptimal Code

5. NearDecoder.sol (4229cd7)

5.1 Suboptimal Code

5.2 Other Issues

6. Borsh.sol (6993b73)

6.1 Critical Flaws

6.2 Arithmetic Overflow Issues

6.3 Suboptimal Code

6.4 Unclear Behaviour

6.5 Other Issues

7. NearBridge.sol (4031aa8)

7.1 Moderate Flaws

7.4 Suboptimal Code

7.5 Unclear Behaviour

7.6 Other Issues

8. INearBridge.sol (1b1f5fd)

8.1 Documentation Issues

8.2 Other Issues

Summary