Feature/spear bits audit5 #90

invocamanman · 2023-01-27T12:36:49Z

This PR solves the following spearbits audit issues:
https://github.com/spearbit-audits/review-polygon-zkevm-contracts/issues/42
https://github.com/spearbit-audits/review-polygon-zkevm-contracts/issues/43
https://github.com/spearbit-audits/review-polygon-zkevm-contracts/issues/46
https://github.com/spearbit-audits/review-polygon-zkevm-contracts/issues/22
https://github.com/spearbit-audits/review-polygon-zkevm-contracts/issues/15
https://github.com/spearbit-audits/review-polygon-zkevm-contracts/issues/38

Also this PR have the following changes:

Erase not used imports in PolygonZkEVM contract
Update a little bit the condition on DepositContract:
- Before: depositCount < _MAX_DEPOSIT_COUNT
- After: depositCount > _MAX_DEPOSIT_COUNT
  Note that now depositCount can be ==_MAX_DEPOSIT_COUNT, this should be safe and it's more consistent wit the use of MAX in the whole contract.
Update a little bit the logic of PolygonZkEVMGlobalExitRoot::updateExitRoot after using custom errors to optimize gas
The change from < to > in setVerifyBatchTimeTarget / newVerifyBatchTimeTarget but the difference is neglectable PolygonZkEVM.sol#L1224

0xleastwood · 2023-02-02T20:49:57Z

contracts/PolygonZkEVM.sol

-            newVerifyBatchTimeTarget < 1 days,
-            "PolygonZkEVM::setVerifyBatchTimeTarget: setVerifyBatchTimeTarget incorrect range"
-        );
+        if (newVerifyBatchTimeTarget > 1 days) {


Is this a conscious choice to save a little bit of gas by not doing newVerifyBatchTimeTarget >= 1 days?

difference here would be negligible as already discussed in https://github.com/spearbit-audits/review-polygon-zkevm-contracts/issues/22

Yes! it is!

0xleastwood · 2023-02-02T20:58:58Z

lgtm!

0xleastwood · 2023-02-02T21:00:07Z

contracts/PolygonZkEVMBridge.sol

+        (bool success, bytes memory data) = address(token).staticcall(
+            abi.encodeCall(IERC20MetadataUpgradeable.decimals, ())
+        );
+        return success && data.length == 32 ? uint8(abi.decode(data, (uint256))) : 18;


Might make sense to use uint8 here and let it revert for tokens that use decimals greater than 255.

yee i will change it again! i agree

0xleastwood · 2023-02-02T21:07:36Z

contracts/lib/DepositContract.sol

-            depositCount < _MAX_DEPOSIT_COUNT,
-            "DepositContract:_deposit: Merkle tree full"
-        );
+        if (depositCount > _MAX_DEPOSIT_COUNT) {


Regarding this as mentioned in https://github.com/spearbit-audits/review-polygon-zkevm-contracts/issues/22 , this would allow for _MAX_DEPOSIT_COUNT + 1 deposits which I think would overflow the merkle tree?

EDIT: This would not overflow the merkle tree but instead make use of the last empty leaf represented by 32 zero bits.

The bit representation for _MAX_DEPOSIT_COUNT + 1 will be identical to the zero deposit when we look only at the right most 32 bits. But fortunately, we increment depositCount the first time this function is called, so we don't already use this leaf and hence, nothing should be overwritten as far as I can see.

Yes, this was a little bit gas saving, the last deposit if i'm not mistaken will happen something like this:
depositCount = 2**32 --> the first 32 bits will be 0

uint256 size = ++depositCount; for ( uint256 height = 0; height < _DEPOSIT_CONTRACT_TREE_DEPTH; height++ ) { if (((size >> height) & 1) == 1) { _branch[height] = node; return; } node = keccak256(abi.encodePacked(_branch[height], node)); }

Here the condition if (((size >> height) & 1) == 1) will never be true, and therefore we will get into the assert(false)

Anyway this does not seem obvious so i will just put the equal in the revert condition

Ye I agree!

krlosMata · 2023-02-02T21:11:49Z

contracts/interfaces/IPolygonZkEVMBridge.sol

+    error EtherTransferFailed();
+
+    /**
+     * @dev Thrown when the message transactoin on claimMessage fail


Suggested change

* @dev Thrown when the message transactoin on claimMessage fail

* @dev Thrown when the message transaction on claimMessage fail

krlosMata · 2023-02-02T21:12:18Z

contracts/interfaces/IPolygonZkEVMBridge.sol

+    error GlobalExitRootInvalid();
+
+    /**
+     * @dev Thrown when a the smt proof does not match


Suggested change

* @dev Thrown when a the smt proof does not match

* @dev Thrown when the smt proof does not match

krlosMata · 2023-02-02T21:13:03Z

contracts/interfaces/IPolygonZkEVMBridge.sol

+    error NotValidOwner();
+
+    /**
+     * @dev Thrown when the spender of permit does not this contract


Suggested change

* @dev Thrown when the spender of permit does not this contract

* @dev Thrown when the spender of permit is not this contract

krlosMata · 2023-02-02T21:14:52Z

contracts/interfaces/IPolygonZkEVMErrors.sol

+    error OnlyTrustedAggregator();
+
+    /**
+     * @dev Thrown when try to sequence 0 batches


Suggested change

* @dev Thrown when try to sequence 0 batches

* @dev Thrown when trying to sequence 0 batches

krlosMata · 2023-02-02T21:15:12Z

contracts/interfaces/IPolygonZkEVMErrors.sol

+    error SequenceZeroBatches();
+
+    /**
+     * @dev Thrown when an try to sequence or verify more batches than _MAX_VERIFY_BATCHES


Suggested change

* @dev Thrown when an try to sequence or verify more batches than _MAX_VERIFY_BATCHES

* @dev Thrown when trying to sequence or verify more batches than _MAX_VERIFY_BATCHES

krlosMata · 2023-02-02T21:28:02Z

contracts/mocks/ERC20WeirdMetadata.sol

+pragma solidity ^0.8.0;
+
+/**
+ * @dev Implementation of the ERC20 with werid metadata.


Suggested change

* @dev Implementation of the ERC20 with werid metadata.

* @dev Implementation of the ERC20 with weird metadata.

krlosMata · 2023-02-02T21:45:28Z

contracts/PolygonZkEVMBridge.sol

+        (bool success, bytes memory data) = address(token).staticcall(
+            abi.encodeCall(IERC20MetadataUpgradeable.symbol, ())
+        );
+        return success ? _returnDataToString(data) : "???";


rename ???

Also update NatSpec after change

krlosMata · 2023-02-02T21:45:40Z

contracts/PolygonZkEVMBridge.sol

+        (bool success, bytes memory data) = address(token).staticcall(
+            abi.encodeCall(IERC20MetadataUpgradeable.name, ())
+        );
+        return success ? _returnDataToString(data) : "???";


rename ???

Also update NatSpec after change

krlosMata · 2023-02-02T21:46:08Z

contracts/PolygonZkEVMBridge.sol

+            }
+
+            // If the first one is 0, we do not handle the encoding
+            if (nonZeroBytes == 0) {


not needed to handle

Finally i will handle it, in my opinoin if the first byte returned is 0 means that the token was trying encode somehow a name, i think it's better this way that just an empty string

krlosMata · 2023-02-02T21:46:28Z

contracts/PolygonZkEVMBridge.sol

+            }
+            return string(bytesArray);
+        } else {
+            return "???";


rename ???

pashov · 2023-02-02T22:51:08Z

contracts/interfaces/IPolygonZkEVMErrors.sol

+    error ForceBatchesOverflow();
+
+    /**
+     * @dev Thrown when are sequenced more force batches than actually were submitted


Suggested change

* @dev Thrown when are sequenced more force batches than actually were submitted

* @dev Thrown when more force batches were sequenced than actually submitted

pashov · 2023-02-02T22:52:56Z

contracts/interfaces/IPolygonZkEVMErrors.sol

+    error TrustedAggregatorTimeoutNotExpired();
+
+    /**
+     * @dev Thrown when try to access a pending that does not exist


Suggested change

* @dev Thrown when try to access a pending that does not exist

* @dev Thrown when trying to access a pending state that does not exist

pashov · 2023-02-02T22:53:27Z

contracts/interfaces/IPolygonZkEVMErrors.sol

+
+    /**
+     * @dev Thrown when try to sequence a force batch using sequenceForceBatches and the
+     * force timeout do not expired


Suggested change

* force timeout do not expired

* force timeout did not expire

pashov · 2023-02-02T22:55:19Z

contracts/lib/DepositContract.sol

@@ -8,6 +8,11 @@ import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 * Based on the implementation of the deposit eth2.0 contract https://github.com/ethereum/consensus-specs/blob/dev/solidity_deposit_contract/deposit_contract.sol
 */
 contract DepositContract is Initializable {
+    /**
+     * @dev Thrown when the merle tree is full


Suggested change

* @dev Thrown when the merle tree is full

* @dev Thrown when the merkle tree is full

pashov · 2023-02-02T22:55:57Z

contracts/interfaces/IPolygonZkEVMBridge.sol

+    error MsgValueNotZero();
+
+    /**
+     * @dev Thrown when the ether transfer on claimAsset fail


Suggested change

* @dev Thrown when the ether transfer on claimAsset fail

* @dev Thrown when the ether transfer on claimAsset fails

pashov · 2023-02-02T23:06:26Z

contracts/PolygonZkEVMBridge.sol

+        (bool success, bytes memory data) = address(token).staticcall(
+            abi.encodeCall(IERC20MetadataUpgradeable.symbol, ())
+        );
+        return success ? _returnDataToString(data) : "???";


Also update NatSpec after change

pashov · 2023-02-02T23:06:54Z

contracts/PolygonZkEVMBridge.sol

+        (bool success, bytes memory data) = address(token).staticcall(
+            abi.encodeCall(IERC20MetadataUpgradeable.name, ())
+        );
+        return success ? _returnDataToString(data) : "???";


Also update NatSpec after change

optimization update batch fee

3d4a851

invocamanman requested review from eduadiez, ignasirv and krlosMata January 27, 2023 12:36

cla-bot bot added the cla-signed label Jan 27, 2023

invocamanman added 4 commits January 27, 2023 13:44

optimized update batch fee

2cc8b60

fix update batches gas limit usage

54b4b55

fix comment

99666fe

min max batchfee

80cbd55

invocamanman force-pushed the feature/spearBitsAudit5 branch from b98f765 to 80cbd55 Compare January 30, 2023 16:28

invocamanman changed the base branch from main to develop January 30, 2023 16:29

custom errors

144623a

invocamanman marked this pull request as ready for review February 2, 2023 00:11

safe metadata

1080719

invocamanman force-pushed the feature/spearBitsAudit5 branch from b59d2ff to 1080719 Compare February 2, 2023 01:35

invocamanman added 2 commits February 2, 2023 02:53

add unchecked optimization

29c4200

one require left

31a74f0

invocamanman force-pushed the feature/spearBitsAudit5 branch from 98c5a0d to 31a74f0 Compare February 2, 2023 12:21

add metadata weird cases

9281ba3

0xleastwood reviewed Feb 2, 2023

View reviewed changes

krlosMata requested changes Feb 2, 2023

View reviewed changes

small fixes

94bddf0

pashov reviewed Feb 2, 2023

View reviewed changes

last fixes

f5bb381

invocamanman force-pushed the feature/spearBitsAudit5 branch from d773635 to f5bb381 Compare February 3, 2023 14:07

invocamanman merged commit e242081 into develop Feb 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/spear bits audit5 #90

Feature/spear bits audit5 #90

invocamanman commented Jan 27, 2023 •

edited

Loading

0xleastwood Feb 2, 2023

0xleastwood Feb 2, 2023 •

edited

Loading

invocamanman Feb 2, 2023

0xleastwood commented Feb 2, 2023

0xleastwood Feb 2, 2023

invocamanman Feb 2, 2023

0xleastwood Feb 2, 2023 •

edited

Loading

0xleastwood Feb 2, 2023

invocamanman Feb 2, 2023

0xleastwood Feb 3, 2023

krlosMata Feb 2, 2023

krlosMata Feb 2, 2023

krlosMata Feb 2, 2023

krlosMata Feb 2, 2023

krlosMata Feb 2, 2023

krlosMata Feb 2, 2023

krlosMata Feb 2, 2023

pashov Feb 2, 2023

krlosMata Feb 2, 2023

pashov Feb 2, 2023

krlosMata Feb 2, 2023

invocamanman Feb 3, 2023 •

edited

Loading

krlosMata Feb 2, 2023

pashov Feb 2, 2023

pashov Feb 2, 2023

pashov Feb 2, 2023

pashov Feb 2, 2023

pashov Feb 2, 2023

pashov Feb 2, 2023

pashov Feb 2, 2023

	* @dev Thrown when the message transactoin on claimMessage fail
	* @dev Thrown when the message transaction on claimMessage fail

	* @dev Thrown when a the smt proof does not match
	* @dev Thrown when the smt proof does not match

	* @dev Thrown when the spender of permit does not this contract
	* @dev Thrown when the spender of permit is not this contract

	* @dev Thrown when try to sequence 0 batches
	* @dev Thrown when trying to sequence 0 batches

	* @dev Thrown when an try to sequence or verify more batches than _MAX_VERIFY_BATCHES
	* @dev Thrown when trying to sequence or verify more batches than _MAX_VERIFY_BATCHES

	* @dev Implementation of the ERC20 with werid metadata.
	* @dev Implementation of the ERC20 with weird metadata.

	* @dev Thrown when are sequenced more force batches than actually were submitted
	* @dev Thrown when more force batches were sequenced than actually submitted

	* @dev Thrown when try to access a pending that does not exist
	* @dev Thrown when trying to access a pending state that does not exist

	* force timeout do not expired
	* force timeout did not expire

	* @dev Thrown when the merle tree is full
	* @dev Thrown when the merkle tree is full

	* @dev Thrown when the ether transfer on claimAsset fail
	* @dev Thrown when the ether transfer on claimAsset fails

Feature/spear bits audit5 #90

Feature/spear bits audit5 #90

Conversation

invocamanman commented Jan 27, 2023 • edited Loading

Choose a reason for hiding this comment

0xleastwood Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xleastwood commented Feb 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xleastwood Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

invocamanman Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

invocamanman commented Jan 27, 2023 •

edited

Loading

0xleastwood Feb 2, 2023 •

edited

Loading

0xleastwood Feb 2, 2023 •

edited

Loading

invocamanman Feb 3, 2023 •

edited

Loading