Platform: Code4rena
Start Date: 03/05/2022
Pot Size: $75,000 USDC
Total HM: 6
Participants: 55
Period: 7 days
Judge: Albert Chon
Total Solo HM: 2
Id: 116
League: COSMOS
Rank: 5/55
Findings: 3
Award: $5,500.94
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: p_crypt0
Also found by: 0x1337, GermanKuber, IllIllI, WatchPug, csanuragjain, danb, dirk_y, kirk-baird
The Cudos Network is a special-purpose blockchain designed to provide high-performance, trustless, and permissionless cloud computing for all.
To be considered trustless, both the incentives and the code must be aligned to prevent the possibility of maliciousness, especially by actors of the system given specific powers.
Administrators are able to rug all funds in the gravity bridge, which requires a lot of trust
File: solidity/contracts/Gravity.sol #1 632 function withdrawERC20( 633 address _tokenAddress) 634 external { 635 require(cudosAccessControls.hasAdminRole(msg.sender), "Recipient is not an admin"); 636 uint256 totalBalance = IERC20(_tokenAddress).balanceOf(address(this)); 637 IERC20(_tokenAddress).safeTransfer(msg.sender , totalBalance); 638 }
If this function were able to withdraw to a pre-defined address, or the function required a specific amount to be specified, then under the right circumstances, maybe it would be ok, but the fact that it goes directly to the message sender, and only the full balance can be transferred, and the bridge's token can be taken, makes it seem like this is just a rug vector waiting to be applied. This amount of privilege is excessive.
Code inspection
Only allow withdrawal to a multisig after a timelock has expired, and only of specific amounts. Also, consider making the bridge pausable to mitigate attacks
#0 - mlukanova
2022-05-11T17:04:40Z
Duplicate of #14
🌟 Selected for report: IllIllI
Also found by: 0x1337, 0x1f8b, 0xDjango, 0xkatana, AmitN, CertoraInc, Dravee, Funen, GermanKuber, GimelSec, Hawkeye, JC, MaratCerby, WatchPug, Waze, broccolirob, cccz, ch13fd357r0y3r, cryptphi, danb, defsec, delfin454000, dipp, dirk_y, ellahi, gzeon, hake, hubble, ilan, jah, jayjonah8, kebabsec, kirk-baird, m9800, orion, oyc_109, robee, shenwilly, simon135, sorrynotsorry
1110.8314 USDC - $1,110.83
| Title | Instances | |
|---|---|---|
| 1 | Validator signing address of zero not rejected, allowing anyone to sign | 1 |
| 2 | Unbounded loops may run out of gas | 1 |
| 3 | deployERC20() does not have a reentrancy guard | 1 |
| 4 | Comment does not match the behavior of the code | 2 |
| 5 | abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256() | 1 |
Total: 6 instances over 5 classes (see lower down in this report for the summary table of the Non-critical findings)
ecrecover() returns 0 when the signature does not match. If the validators approve a valset including an address of 0, then anyone will be able to sign messages for that signer, since invalid sigatures will return zero, and will match the zero address.
File: solidity/contracts/Gravity.sol #1 185 return _signer == ecrecover(messageDigest, _v, _r, _s);
The call to ecrecover() costs 3000 gas per call, and if there are too many validators, the update of the validator set may pass, but large batches will fail
File: solidity/contracts/Gravity.sol #1 219 function checkValidatorSignatures( 220 // The current validator set and their powers 221 address[] memory _currentValidators, 222 uint256[] memory _currentPowers, 223 // The current validator's signatures 224 uint8[] memory _v, 225 bytes32[] memory _r, 226 bytes32[] memory _s, 227 // This is what we are checking they have signed 228 bytes32 _theHash, 229 uint256 _powerThreshold 230 ) private pure { 231 uint256 cumulativePower = 0; 232 233 for (uint256 i = 0; i < _currentValidators.length; i++) { 234 // If v is set to 0, this signifies that it was not possible to get a signature from this validator and we skip evaluation 235 // (In a valid signature, it is either 27 or 28) 236 if (_v[i] != 0) { 237 // Check that the current validator has signed off on the hash 238 require( 239 verifySig(_currentValidators[i], _theHash, _v[i], _r[i], _s[i]),
deployERC20() does not have a reentrancy guarddeployERC20() increments the state_lastEventNonce so it's possible for the nonce to be incremented by a transfer hook. I don't see a way to exploit this given the code in scope, but perhaps some other area relies on event nonces happening in a specific order in relation to the other events.
File: solidity/contracts/Gravity.sol #1 611 function deployERC20( 612 string memory _cosmosDenom, 613 string memory _name, 614 string memory _symbol, 615 uint8 _decimals 616 ) public { 617 // Deploy an ERC20 with entire supply granted to Gravity.sol 618 CosmosERC20 erc20 = new CosmosERC20(address(this), _name, _symbol, _decimals); 619 620 // Fire an event to let the Cosmos module know 621 state_lastEventNonce = state_lastEventNonce.add(1);
Both of the functions below have require(isOrchestrator(msg.sender)), and orchestrators are the first signer, so not just anyone can call these
File: solidity/contracts/Gravity.sol #1 362 // Anyone can call this function, but they must supply valid signatures of state_powerThreshold of the current valset over 363 // the batch. 364 function submitBatch (
File: solidity/contracts/Gravity.sol #2 274 // Anyone can call this function, but they must supply valid signatures of state_powerThreshold of the current valset over 275 // the new valset. 276 function updateValset(
abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.
File: solidity/contracts/Gravity.sol #1 182 bytes32 messageDigest = keccak256( 183 abi.encodePacked("\x19Ethereum Signed Message:\n32", _theHash) 184 );
| Title | Instances | |
|---|---|---|
| 1 | Best practice is to prevent signature malleability | 1 |
| 2 | Inconsistent variable naming convention | 2 |
| 3 | Inconsistent tabs vs spaces | 3 |
| 4 | if( should be if ( to match other lines in the file | 1 |
| 5 | Misleading function name | 1 |
| 6 | Avoid the use of sensitive terms in favor of neutral ones | 4 |
| 7 | public functions not called by the contract should be declared external instead | 10 |
| 8 | 2**<n> - 1 should be re-written as type(uint<n>).max | 1 |
| 9 | constants should be defined rather than using magic numbers | 3 |
| 10 | Use a more recent version of solidity | 1 |
| 11 | Variable names that consist of all capital letters should be reserved for const/immutable variables | 1 |
| 12 | Non-library/interface files should use fixed compiler versions, not floating ones | 2 |
| 13 | Typos | 1 |
| 14 | File does not contain an SPDX Identifier | 2 |
| 15 | File is missing NatSpec | 2 |
| 16 | Event is missing indexed fields | 5 |
| 17 | Consider making the bridge 'pausable' | 1 |
Total: 41 instances over 17 classes
Use OpenZeppelin's ECDSA contract rather than calling ecrecover() directly
File: solidity/contracts/Gravity.sol #1 182 bytes32 messageDigest = keccak256( 183 abi.encodePacked("\x19Ethereum Signed Message: 32", _theHash) 184 ); 185 return _signer == ecrecover(messageDigest, _v, _r, _s);
Most state variables use the state_ prefix in their variable name. There are some that don't. Use the prefix everywhere, and manually add public getters where necessary
File: solidity/contracts/Gravity.sol #1 63 CudosAccessControls public cudosAccessControls;
File: solidity/contracts/Gravity.sol #2 65 mapping(address => bool) public whitelisted;
Most lines use tabs, but some use spaces, which leads to alignment issues
File: solidity/contracts/Gravity.sol #1 128 for (uint256 i = 0; i < _users.length; i++) { 129 require( 130 _users[i] != address(0), 131 "User is the zero address" 132 ); 133 whitelisted[_users[i]] = _isWhitelisted; 134 }
File: solidity/contracts/Gravity.sol #2 117 require( 118 whitelisted[msg.sender] || cudosAccessControls.hasAdminRole(msg.sender) , 119 "The caller is not whitelisted for this operation" 120 ); 121 _;
File: solidity/contracts/Gravity.sol #3 647 address[] memory _validators, 648 uint256[] memory _powers, 649 CudosAccessControls _cudosAccessControls
if( should be if ( to match other lines in the fileFile: solidity/contracts/Gravity.sol #1 264 if(_newValset.validators[i] == _sender) {
onlyWhitelisted() should be onlyWhitelistedOrAdmin()
File: solidity/contracts/Gravity.sol #1 116 modifier onlyWhitelisted() {
Use allowlist rather than whitelist
File: solidity/contracts/Gravity.sol #1 116 modifier onlyWhitelisted() {
File: solidity/contracts/Gravity.sol #2 65 mapping(address => bool) public whitelisted;
File: solidity/contracts/Gravity.sol #3 109 event WhitelistedStatusModified(
File: solidity/contracts/Gravity.sol #4 124 function manageWhitelist(
public functions not called by the contract should be declared external insteadContracts are allowed to override their parents' functions and change the visibility from external to public.
File: solidity/contracts/Gravity.sol #1 124 function manageWhitelist( 125 address[] memory _users, 126 bool _isWhitelisted 127 ) public onlyWhitelisted {
File: solidity/contracts/Gravity.sol #2 140 function testMakeCheckpoint(ValsetArgs memory _valsetArgs, bytes32 _gravityId) public pure {
File: solidity/contracts/Gravity.sol #3 144 function testCheckValidatorSignatures( 145 address[] memory _currentValidators, 146 uint256[] memory _currentPowers, 147 uint8[] memory _v, 148 bytes32[] memory _r, 149 bytes32[] memory _s, 150 bytes32 _theHash, 151 uint256 _powerThreshold
File: solidity/contracts/Gravity.sol #4 166 function lastBatchNonce(address _erc20Address) public view returns (uint256) {
File: solidity/contracts/Gravity.sol #5 170 function lastLogicCallNonce(bytes32 _invalidation_id) public view returns (uint256) {
File: solidity/contracts/Gravity.sol #6 276 function updateValset( 277 // The new version of the validator set 278 ValsetArgs memory _newValset, 279 // The current validators that approve the change 280 ValsetArgs memory _currentValset, 281 // These are arrays of the parts of the current validator's signatures 282 uint8[] memory _v, 283 bytes32[] memory _r, 284 bytes32[] memory _s 285 ) public nonReentrant {
File: solidity/contracts/Gravity.sol #7 364 function submitBatch ( 365 // The validators that approve the batch 366 ValsetArgs memory _currentValset, 367 // These are arrays of the parts of the validators signatures 368 uint8[] memory _v, 369 bytes32[] memory _r, 370 bytes32[] memory _s, 371 // The batch of transactions 372 uint256[] memory _amounts, 373 address[] memory _destinations, 374 uint256[] memory _fees, 375 uint256 _batchNonce, 376 address _tokenContract, 377 // a block height beyond which this batch is not valid 378 // used to provide a fee-free timeout 379 uint256 _batchTimeout 380 ) public nonReentrant {
File: solidity/contracts/Gravity.sol #8 479 function submitLogicCall( 480 // The validators that approve the call 481 ValsetArgs memory _currentValset, 482 // These are arrays of the parts of the validators signatures 483 uint8[] memory _v, 484 bytes32[] memory _r, 485 bytes32[] memory _s, 486 LogicCallArgs memory _args 487 ) public nonReentrant {
File: solidity/contracts/Gravity.sol #9 595 function sendToCosmos( 596 address _tokenContract, 597 bytes32 _destination, 598 uint256 _amount 599 ) public nonReentrant {
File: solidity/contracts/Gravity.sol #10 611 function deployERC20( 612 string memory _cosmosDenom, 613 string memory _name, 614 string memory _symbol, 615 uint8 _decimals
2**<n> - 1 should be re-written as type(uint<n>).maxEarlier versions of solidity can use uint<n>(-1) instead. Expressions not including the - 1 can often be re-written to accomodate the change (e.g. by using a > rather than a >=, which will also save some gas)
File: solidity/contracts/CosmosToken.sol #1 5 uint256 MAX_UINT = 2**256 - 1;
constants should be defined rather than using magic numbersFile: solidity/contracts/Gravity.sol #1 202 bytes32 methodName = 0x636865636b706f696e7400000000000000000000000000000000000000000000;
File: solidity/contracts/Gravity.sol #2 433 0x7472616e73616374696f6e426174636800000000000000000000000000000000,
File: solidity/contracts/Gravity.sol #3 535 0x6c6f67696343616c6c0000000000000000000000000000000000000000000000,
Use a solidity version of at least 0.8.4 to get bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>)
Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>)
File: solidity/contracts/Gravity.sol #1 1 pragma solidity ^0.6.6;
const/immutable variablesIf the variable needs to be different based on which class it comes from, a view/pure function should be used instead (e.g. like this).
File: solidity/contracts/CosmosToken.sol #1 5 uint256 MAX_UINT = 2**256 - 1;
File: solidity/contracts/CosmosToken.sol #1 1 pragma solidity ^0.6.6;
File: solidity/contracts/Gravity.sol #2 1 pragma solidity ^0.6.6;
File: solidity/contracts/Gravity.sol #1 564 // Update invaldiation nonce
File: solidity/contracts/CosmosToken.sol #1 0 pragma solidity ^0.6.6;
File: solidity/contracts/Gravity.sol #2 0 pragma solidity ^0.6.6;
File: solidity/contracts/CosmosToken.sol (various lines) #1
File: solidity/contracts/Gravity.sol (various lines) #2
indexed fieldsEach event should use three indexed fields if there are three or more fields
File: solidity/contracts/Gravity.sol #1 73 event TransactionBatchExecutedEvent( 74 uint256 indexed _batchNonce, 75 address indexed _token, 76 uint256 _eventNonce 77 );
File: solidity/contracts/Gravity.sol #2 85 event ERC20DeployedEvent( 86 // FYI: Can't index on a string without doing a bunch of weird stuff 87 string _cosmosDenom, 88 address indexed _tokenContract, 89 string _name, 90 string _symbol, 91 uint8 _decimals, 92 uint256 _eventNonce 93 );
File: solidity/contracts/Gravity.sol #3 94 event ValsetUpdatedEvent( 95 uint256 indexed _newValsetNonce, 96 uint256 _eventNonce, 97 uint256 _rewardAmount, 98 address _rewardToken, 99 address[] _validators, 100 uint256[] _powers 101 );
File: solidity/contracts/Gravity.sol #4 102 event LogicCallEvent( 103 bytes32 _invalidationId, 104 uint256 _invalidationNonce, 105 bytes _returnData, 106 uint256 _eventNonce 107 );
File: solidity/contracts/Gravity.sol #5 109 event WhitelistedStatusModified( 110 address _sender, 111 address[] _users, 112 bool _isWhitelisted 113 );
Having this ability would help to mitigate attacks and would ameleorate the need for this withdrawERC20() to be all-or-nothing
File: solidity/contracts/Gravity.sol #1 632 function withdrawERC20( 633 address _tokenAddress) 634 external { 635 require(cudosAccessControls.hasAdminRole(msg.sender), "Recipient is not an admin"); 636 uint256 totalBalance = IERC20(_tokenAddress).balanceOf(address(this)); 637 IERC20(_tokenAddress).safeTransfer(msg.sender , totalBalance); 638 }
#0 - V-Staykov
2022-05-10T15:22:31Z
This is a particularly high quality
🌟 Selected for report: GermanKuber
Also found by: 0v3rf10w, 0x1f8b, 0xDjango, 0xNazgul, 0xf15ers, 0xkatana, AlleyCat, CertoraInc, Dravee, Funen, GimelSec, IllIllI, JC, MaratCerby, WatchPug, Waze, defsec, delfin454000, ellahi, gzeon, hake, hansfriese, ilan, jonatascm, nahnah, oyc_109, peritoflores, rfa, robee, simon135, slywaters, sorrynotsorry
385.8235 USDC - $385.82
| Title | Instances | |
|---|---|---|
| 1 | State variables only set in the constructor should be declared immutable | 3 |
| 2 | State variables should be cached in stack variables rather than re-reading them from storage | 8 |
| 3 | <array>.length should not be looked up in every loop of a for-loop | 7 |
| 4 | require()/revert() strings longer than 32 bytes cost extra gas | 16 |
| 5 | Using bools for storage incurs overhead | 1 |
| 6 | Use a more recent version of solidity | 2 |
| 7 | It costs more gas to initialize variables to zero than to let the default of zero be applied | 9 |
| 8 | ++i costs less gas than ++i, especially when it's used in for-loops (--i/i-- too) | 7 |
| 9 | Splitting require() statements that use && saves gas | 4 |
| 10 | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead | 4 |
| 11 | Duplicated require()/revert() checks should be refactored to a modifier or function | 4 |
| 12 | Functions guaranteed to revert when called by normal users can be marked payable | 1 |
| 13 | public functions not called by the contract should be declared external instead | 10 |
| 14 | Return from function rather than breaking out of loop | 1 |
| 15 | require() or revert() statements that check input arguments should be at the top of the function | 3 |
| 16 | Remove test code to save deployment gas | 1 |
Total: 81 instances over 16 classes
immutableAvoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).
File: solidity/contracts/Gravity.sol #1 60 bytes32 public state_gravityId;
File: solidity/contracts/Gravity.sol #2 61 uint256 public state_powerThreshold;
File: solidity/contracts/Gravity.sol #3 63 CudosAccessControls public cudosAccessControls;
The instances below point to the second+ access of a state variable within a function. Caching will replace each Gwarmaccess (100 gas) with a much cheaper stack read. Less obvious fixes/optimizations include having local storage variables of mappings within state variable mappings or mappings within state variable structs, having local storage variables of structs within mappings, having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
File: solidity/contracts/Gravity.sol #1 352 state_lastEventNonce,
state_lastEventNonce https://github.com/code-423n4/2022-05-cudos/blob/de39cf3cd1f1e1cf211819b06d4acf6a043acda0/solidity/contracts/Gravity.sol#L352
File: solidity/contracts/Gravity.sol #2 466 emit TransactionBatchExecutedEvent(_batchNonce, _tokenContract, state_lastEventNonce);
state_lastEventNonce https://github.com/code-423n4/2022-05-cudos/blob/de39cf3cd1f1e1cf211819b06d4acf6a043acda0/solidity/contracts/Gravity.sol#L466
File: solidity/contracts/Gravity.sol #3 590 state_lastEventNonce
state_lastEventNonce https://github.com/code-423n4/2022-05-cudos/blob/de39cf3cd1f1e1cf211819b06d4acf6a043acda0/solidity/contracts/Gravity.sol#L590
File: solidity/contracts/Gravity.sol #4 607 state_lastEventNonce
state_lastEventNonce https://github.com/code-423n4/2022-05-cudos/blob/de39cf3cd1f1e1cf211819b06d4acf6a043acda0/solidity/contracts/Gravity.sol#L607
File: solidity/contracts/Gravity.sol #5 628 state_lastEventNonce
state_lastEventNonce https://github.com/code-423n4/2022-05-cudos/blob/de39cf3cd1f1e1cf211819b06d4acf6a043acda0/solidity/contracts/Gravity.sol#L628
File: solidity/contracts/Gravity.sol #6 321 bytes32 newCheckpoint = makeCheckpoint(_newValset, state_gravityId);
state_gravityId https://github.com/code-423n4/2022-05-cudos/blob/de39cf3cd1f1e1cf211819b06d4acf6a043acda0/solidity/contracts/Gravity.sol#L321
File: solidity/contracts/Gravity.sol #7 431 state_gravityId,
state_gravityId https://github.com/code-423n4/2022-05-cudos/blob/de39cf3cd1f1e1cf211819b06d4acf6a043acda0/solidity/contracts/Gravity.sol#L431
File: solidity/contracts/Gravity.sol #8 533 state_gravityId,
state_gravityId https://github.com/code-423n4/2022-05-cudos/blob/de39cf3cd1f1e1cf211819b06d4acf6a043acda0/solidity/contracts/Gravity.sol#L533
<array>.length should not be looked up in every loop of a for-loopThe overheads outlined below are PER LOOP, excluding the first loop
MLOAD (3 gas)CALLDATALOAD (3 gas)Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset
File: solidity/contracts/Gravity.sol #1 128 for (uint256 i = 0; i < _users.length; i++) {
File: solidity/contracts/Gravity.sol #2 233 for (uint256 i = 0; i < _currentValidators.length; i++) {
File: solidity/contracts/Gravity.sol #3 263 for (uint256 i = 0; i < _newValset.validators.length; i++) {
File: solidity/contracts/Gravity.sol #4 453 for (uint256 i = 0; i < _amounts.length; i++) {
File: solidity/contracts/Gravity.sol #5 568 for (uint256 i = 0; i < _args.transferAmounts.length; i++) {
File: solidity/contracts/Gravity.sol #6 579 for (uint256 i = 0; i < _args.feeAmounts.length; i++) {
File: solidity/contracts/Gravity.sol #7 660 for (uint256 i = 0; i < _powers.length; i++) {
require()/revert() strings longer than 32 bytes cost extra gasFile: solidity/contracts/Gravity.sol #1 117 require( 118 whitelisted[msg.sender] || cudosAccessControls.hasAdminRole(msg.sender) , 119 "The caller is not whitelisted for this operation" 120 );
File: solidity/contracts/Gravity.sol #2 238 require( 239 verifySig(_currentValidators[i], _theHash, _v[i], _r[i], _s[i]), 240 "Validator signature does not match." 241 );
File: solidity/contracts/Gravity.sol #3 254 require( 255 cumulativePower > _powerThreshold, 256 "Submitted validator set signatures do not have enough power." 257 );
File: solidity/contracts/Gravity.sol #4 289 require( 290 _newValset.valsetNonce > _currentValset.valsetNonce, 291 "New valset nonce must be greater than the current nonce" 292 );
File: solidity/contracts/Gravity.sol #5 310 require( 311 makeCheckpoint(_currentValset, state_gravityId) == state_lastValsetCheckpoint, 312 "Supplied current validators and powers do not match checkpoint." 313 );
File: solidity/contracts/Gravity.sol #6 315 require( 316 isOrchestrator(_currentValset, msg.sender), 317 "The sender of the transaction is not validated orchestrator" 318 );
File: solidity/contracts/Gravity.sol #7 384 require( 385 state_lastBatchNonces[_tokenContract] < _batchNonce, 386 "New batch nonce must be greater than the current nonce" 387 );
File: solidity/contracts/Gravity.sol #8 390 require( 391 block.number < _batchTimeout, 392 "Batch timeout must be greater than the current block height" 393 );
File: solidity/contracts/Gravity.sol #9 405 require( 406 makeCheckpoint(_currentValset, state_gravityId) == state_lastValsetCheckpoint, 407 "Supplied current validators and powers do not match checkpoint." 408 );
File: solidity/contracts/Gravity.sol #10 416 require( 417 isOrchestrator(_currentValset, msg.sender), 418 "The sender of the transaction is not validated orchestrator" 419 );
File: solidity/contracts/Gravity.sol #11 494 require( 495 state_invalidationMapping[_args.invalidationId] < _args.invalidationNonce, 496 "New invalidation nonce must be greater than the current nonce" 497 );
File: solidity/contracts/Gravity.sol #12 509 require( 510 makeCheckpoint(_currentValset, state_gravityId) == state_lastValsetCheckpoint, 511 "Supplied current validators and powers do not match checkpoint." 512 );
File: solidity/contracts/Gravity.sol #13 515 require( 516 _args.transferAmounts.length == _args.transferTokenContracts.length, 517 "Malformed list of token transfers" 518 );
File: solidity/contracts/Gravity.sol #14 525 require( 526 isOrchestrator(_currentValset, msg.sender), 527 "The sender of the transaction is not validated orchestrator" 528 );
File: solidity/contracts/Gravity.sol #15 655 require(address(_cudosAccessControls) != address(0), "Access control contract address is incorrect");
File: solidity/contracts/Gravity.sol #16 666 require( 667 cumulativePower > _powerThreshold, 668 "Submitted validator set signatures do not have enough power." 669 );
bools for storage incurs overhead// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27
Use uint256(1) and uint256(2) for true/false
File: solidity/contracts/Gravity.sol #1 65 mapping(address => bool) public whitelisted;
Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath
Use a solidity version of at least 0.8.2 to get compiler automatic inlining
Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads
Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings
Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
File: solidity/contracts/CosmosToken.sol #1 1 pragma solidity ^0.6.6;
File: solidity/contracts/Gravity.sol #2 1 pragma solidity ^0.6.6;
File: solidity/contracts/Gravity.sol #1 128 for (uint256 i = 0; i < _users.length; i++) {
File: solidity/contracts/Gravity.sol #2 231 uint256 cumulativePower = 0;
File: solidity/contracts/Gravity.sol #3 233 for (uint256 i = 0; i < _currentValidators.length; i++) {
File: solidity/contracts/Gravity.sol #4 263 for (uint256 i = 0; i < _newValset.validators.length; i++) {
File: solidity/contracts/Gravity.sol #5 453 for (uint256 i = 0; i < _amounts.length; i++) {
File: solidity/contracts/Gravity.sol #6 568 for (uint256 i = 0; i < _args.transferAmounts.length; i++) {
File: solidity/contracts/Gravity.sol #7 579 for (uint256 i = 0; i < _args.feeAmounts.length; i++) {
File: solidity/contracts/Gravity.sol #8 659 uint256 cumulativePower = 0;
File: solidity/contracts/Gravity.sol #9 660 for (uint256 i = 0; i < _powers.length; i++) {
++i costs less gas than ++i, especially when it's used in for-loops (--i/i-- too)Saves 6 gas PER LOOP
File: solidity/contracts/Gravity.sol #1 128 for (uint256 i = 0; i < _users.length; i++) {
File: solidity/contracts/Gravity.sol #2 233 for (uint256 i = 0; i < _currentValidators.length; i++) {
File: solidity/contracts/Gravity.sol #3 263 for (uint256 i = 0; i < _newValset.validators.length; i++) {
File: solidity/contracts/Gravity.sol #4 453 for (uint256 i = 0; i < _amounts.length; i++) {
File: solidity/contracts/Gravity.sol #5 568 for (uint256 i = 0; i < _args.transferAmounts.length; i++) {
File: solidity/contracts/Gravity.sol #6 579 for (uint256 i = 0; i < _args.feeAmounts.length; i++) {
File: solidity/contracts/Gravity.sol #7 660 for (uint256 i = 0; i < _powers.length; i++) {
require() statements that use && saves gasSee this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper
File: solidity/contracts/Gravity.sol #1 301 require( 302 _currentValset.validators.length == _currentValset.powers.length && 303 _currentValset.validators.length == _v.length && 304 _currentValset.validators.length == _r.length && 305 _currentValset.validators.length == _s.length, 306 "Malformed current validator set" 307 );
File: solidity/contracts/Gravity.sol #2 396 require( 397 _currentValset.validators.length == _currentValset.powers.length && 398 _currentValset.validators.length == _v.length && 399 _currentValset.validators.length == _r.length && 400 _currentValset.validators.length == _s.length, 401 "Malformed current validator set" 402 );
File: solidity/contracts/Gravity.sol #3 411 require( 412 _amounts.length == _destinations.length && _amounts.length == _fees.length, 413 "Malformed batch of transactions" 414 );
File: solidity/contracts/Gravity.sol #4 500 require( 501 _currentValset.validators.length == _currentValset.powers.length && 502 _currentValset.validators.length == _v.length && 503 _currentValset.validators.length == _r.length && 504 _currentValset.validators.length == _s.length, 505 "Malformed current validator set" 506 );
uints/ints smaller than 32 bytes (256 bits) incurs overheadWhen using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed
File: solidity/contracts/CosmosToken.sol #1 11 uint8 _decimals
File: solidity/contracts/Gravity.sol #2 91 uint8 _decimals,
File: solidity/contracts/Gravity.sol #3 178 uint8 _v,
File: solidity/contracts/Gravity.sol #4 615 uint8 _decimals
require()/revert() checks should be refactored to a modifier or functionSaves deployment costs
File: solidity/contracts/Gravity.sol #1 666 require( 667 cumulativePower > _powerThreshold, 668 "Submitted validator set signatures do not have enough power." 669 );
File: solidity/contracts/Gravity.sol #2 396 require( 397 _currentValset.validators.length == _currentValset.powers.length && 398 _currentValset.validators.length == _v.length && 399 _currentValset.validators.length == _r.length && 400 _currentValset.validators.length == _s.length, 401 "Malformed current validator set" 402 );
File: solidity/contracts/Gravity.sol #3 405 require( 406 makeCheckpoint(_currentValset, state_gravityId) == state_lastValsetCheckpoint, 407 "Supplied current validators and powers do not match checkpoint." 408 );
File: solidity/contracts/Gravity.sol #4 416 require( 417 isOrchestrator(_currentValset, msg.sender), 418 "The sender of the transaction is not validated orchestrator" 419 );
payableIf a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are
CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost
File: solidity/contracts/Gravity.sol #1 124 function manageWhitelist( 125 address[] memory _users, 126 bool _isWhitelisted 127 ) public onlyWhitelisted {
public functions not called by the contract should be declared external insteadContracts are allowed to override their parents' functions and change the visibility from external to public and can save gas by doing so.
File: solidity/contracts/Gravity.sol #1 124 function manageWhitelist( 125 address[] memory _users, 126 bool _isWhitelisted 127 ) public onlyWhitelisted {
File: solidity/contracts/Gravity.sol #2 140 function testMakeCheckpoint(ValsetArgs memory _valsetArgs, bytes32 _gravityId) public pure {
File: solidity/contracts/Gravity.sol #3 144 function testCheckValidatorSignatures( 145 address[] memory _currentValidators, 146 uint256[] memory _currentPowers, 147 uint8[] memory _v, 148 bytes32[] memory _r, 149 bytes32[] memory _s, 150 bytes32 _theHash, 151 uint256 _powerThreshold
File: solidity/contracts/Gravity.sol #4 166 function lastBatchNonce(address _erc20Address) public view returns (uint256) {
File: solidity/contracts/Gravity.sol #5 170 function lastLogicCallNonce(bytes32 _invalidation_id) public view returns (uint256) {
File: solidity/contracts/Gravity.sol #6 276 function updateValset( 277 // The new version of the validator set 278 ValsetArgs memory _newValset, 279 // The current validators that approve the change 280 ValsetArgs memory _currentValset, 281 // These are arrays of the parts of the current validator's signatures 282 uint8[] memory _v, 283 bytes32[] memory _r, 284 bytes32[] memory _s 285 ) public nonReentrant {
File: solidity/contracts/Gravity.sol #7 364 function submitBatch ( 365 // The validators that approve the batch 366 ValsetArgs memory _currentValset, 367 // These are arrays of the parts of the validators signatures 368 uint8[] memory _v, 369 bytes32[] memory _r, 370 bytes32[] memory _s, 371 // The batch of transactions 372 uint256[] memory _amounts, 373 address[] memory _destinations, 374 uint256[] memory _fees, 375 uint256 _batchNonce, 376 address _tokenContract, 377 // a block height beyond which this batch is not valid 378 // used to provide a fee-free timeout 379 uint256 _batchTimeout 380 ) public nonReentrant {
File: solidity/contracts/Gravity.sol #8 479 function submitLogicCall( 480 // The validators that approve the call 481 ValsetArgs memory _currentValset, 482 // These are arrays of the parts of the validators signatures 483 uint8[] memory _v, 484 bytes32[] memory _r, 485 bytes32[] memory _s, 486 LogicCallArgs memory _args 487 ) public nonReentrant {
File: solidity/contracts/Gravity.sol #9 595 function sendToCosmos( 596 address _tokenContract, 597 bytes32 _destination, 598 uint256 _amount 599 ) public nonReentrant {
File: solidity/contracts/Gravity.sol #10 611 function deployERC20( 612 string memory _cosmosDenom, 613 string memory _name, 614 string memory _symbol, 615 uint8 _decimals
Using return rather than break saves gas because the require() outside of the for-loop has the same condition that caused the loop to be broken out of
File: solidity/contracts/Gravity.sol #1 246 // Break early to avoid wasting gas 247 if (cumulativePower > _powerThreshold) { 248 break; 249 } 250 } 251 } 252 253 // Check that there was enough power 254 require( 255 cumulativePower > _powerThreshold, 256 "Submitted validator set signatures do not have enough power." 257 ); 258 // Success 259 }
require() or revert() statements that check input arguments should be at the top of the functionChecks that involve constants should come before checks that involve state variables
File: solidity/contracts/Gravity.sol #1 411 require( 412 _amounts.length == _destinations.length && _amounts.length == _fees.length, 413 "Malformed batch of transactions" 414 );
File: solidity/contracts/Gravity.sol #2 514 // Check that the token transfer list is well-formed 515 require( 516 _args.transferAmounts.length == _args.transferTokenContracts.length, 517 "Malformed list of token transfers" 518 );
File: solidity/contracts/Gravity.sol #3 520 // Check that the fee list is well-formed 521 require( 522 _args.feeAmounts.length == _args.feeTokenContracts.length, 523 "Malformed list of fees" 524 );
File: solidity/contracts/Gravity.sol #1 138 // TEST FIXTURES 139 // These are here to make it easier to measure gas usage. They should be removed before production 140 function testMakeCheckpoint(ValsetArgs memory _valsetArgs, bytes32 _gravityId) public pure { 141 makeCheckpoint(_valsetArgs, _gravityId); 142 } 143 144 function testCheckValidatorSignatures( 145 address[] memory _currentValidators, 146 uint256[] memory _currentPowers, 147 uint8[] memory _v, 148 bytes32[] memory _r, 149 bytes32[] memory _s, 150 bytes32 _theHash, 151 uint256 _powerThreshold 152 ) public pure { 153 checkValidatorSignatures( 154 _currentValidators, 155 _currentPowers, 156 _v, 157 _r, 158 _s, 159 _theHash, 160 _powerThreshold 161 ); 162 } 163 164 // END TEST FIXTURES
#0 - V-Staykov
2022-05-10T11:11:43Z
I find this report particularly high quality.