Platform: Code4rena
Start Date: 18/10/2022
Pot Size: $75,000 USDC
Total HM: 27
Participants: 144
Period: 7 days
Judge: gzeon
Total Solo HM: 13
Id: 170
League: ETH
Rank: 72/144
Findings: 2
Award: $26.35
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: Rolezn
Also found by: 0x1f8b, 0x52, 0x5rings, 0xNazgul, 0xSmartContract, 0xZaharina, 0xhunter, 0xzh, 8olidity, Amithuddar, Aymen0909, B2, Bnke0x0, Chom, Deivitto, Diana, Diraco, Dravee, Franfran, JC, Jeiwan, Josiah, JrNet, Jujic, KingNFT, KoKo, Lambda, Margaret, Migue, Ocean_Sky, PaludoX0, Picodes, Rahoz, RaoulSchaffranek, RaymondFam, RedOneN, ReyAdmirado, Shinchan, Tagir2003, Trust, Waze, Yiko, __141345__, a12jmx, adriro, ajtra, arcoun, aysha, ballx, bin2chen, bobirichman, brgltd, bulej93, catchup, catwhiskeys, caventa, cccz, cdahlheimer, ch0bu, chaduke, chrisdior4, cloudjunky, cryptostellar5, cryptphi, csanuragjain, cylzxje, d3e4, delfin454000, djxploit, durianSausage, erictee, fatherOfBlocks, francoHacker, gianganhnguyen, gogo, hansfriese, i_got_hacked, ignacio, imare, karanctf, kv, leosathya, louhk, lukris02, lyncurion, m_Rassska, malinariy, martin, mcwildy, mics, minhtrng, nicobevi, oyc_109, pashov, peanuts, pedr02b2, peiw, rbserver, ret2basic, rotcivegaf, rvierdiiev, ryshaw, sakman, sakshamguruji, saneryee, securerodd, seyni, sikorico, svskaushik, teawaterwire, tnevler, w0Lfrum
0 USDC - $0.00
SECP256K1 signaturesHere, the ecrecover() method doesn't check the s range.
Homestead (EIP-2) added this limitation, however the precompile remained unaltered. The majority of libraries, including OpenZeppelin, do this check.
Also there was no check of the address(0), so using a signer with an empty address can by pass the signature and sign arguments in deployHolographableContract.
Reference:
Affected source code:
The pragma version used are:
pragma solidity 0.8.13;
Note that mixing pragma is not recommended. Because different compiler versions have different meanings and behaviors, it also significantly raises maintenance costs. As a result, depending on the compiler version selected for any given file, deployed contracts may have security issues.
The minimum required version must be 0.8.17; otherwise, contracts will be affected by the following important bug fixes:
calldatasize() in all cases.bytes arrays.Apart from these, there are several minor bug fixes and improvements.
payable unnecessarilyThe following methods have been marked as payable, this may cause both administrators and users calling them to accidentally deposit ether with no recovery possible.
This may be done to save gas, but may result in more loss than benefit in case of one human error.
Affected source code:
abi.encodePacked with dynamic types does not pad the elements, so it is possible to create a keccack collision for attackers if there are multiple string or byte arguments.
Affected source code:
A user supplied argument can be passed as zero to multiplication operation.
Reference:
Affected source code:
The PA1D.getEthPayout, PA1D.getTokenPayout and PA1D.getTokensPayout methods do not have the noReentrant modifier and make calls to an external contract that can take advantage of and call these methods again, but it seems to fail due to the lack of tokens.
However, if any of the other addresses used their receive event to provide liquidity to the contract, the attacking account could benefit from it.
function getEthPayout() public { _validatePayoutRequestor(); _payoutEth(); } /** * @notice Get payout for a specific token address. Token must have a positive balance! * @dev Contract owner, admin, identity wallet, and payout recipients can call this function. * @param tokenAddress An address of the token for which to issue payouts for. */ function getTokenPayout(address tokenAddress) public { _validatePayoutRequestor(); _payoutToken(tokenAddress); } /** * @notice Get payouts for tokens listed by address. Tokens must have a positive balance! * @dev Each token balance must be equal or greater than 10000. Otherwise calculating BP is difficult. * @param tokenAddresses An address array of tokens to issue payouts for. */ function getTokensPayout(address[] memory tokenAddresses) public { _validatePayoutRequestor(); _payoutTokens(tokenAddresses); }
Affected source code:
The comment says: Do not call this function, it will always revert, but that's not true, it won't revert if the external call throw an error, it's better to hide this method and change the visibility to internal becauthe the method contains a call to an external contract:
function revertedBridgeOutRequest( address sender, uint32 toChain, address holographableContract, bytes calldata bridgeOutPayload - ) external returns (string memory revertReason) { + ) internal returns (string memory revertReason) {
Affected source code:
jobNonce was not increasedThe method HolographBridge.getBridgeOutRequestPayload has the following comment:
dev the latest job nonce is incremented by one
But that's not true because it only uses the current jobNonce + 1, it wasn't incremented, next time, it will be exactly the same number. Nothe that it's also not increased during the HolographOperator.crossChainMessage and it could cause the contract to not work as expected, and cause unexpected errors.
Affected source code:
supportsInterfaceThe EIP-165 standard helps detect that a smart contract implements the expected logic, prevents human error when configuring smart contract bindings, so it is recommended to check that the received argument is a contract and supports the expected interface.
Reference:
Affected source code:
abstract method to virtualIt is better to use virtual to improve the extensibility of the contract.
Affected source code:
It is not good practice to hardcode values, but if you are dealing with addresses much less, these can change between implementations, networks or projects, so it is convenient to remove these values from the source code.
Affected source code:
Use the selector instead of the raw value:
🌟 Selected for report: oyc_109
Also found by: 0x040, 0x1f8b, 0x5rings, 0xNazgul, 0xSmartContract, 0xZaharina, 0xsam, 0xzh, 2997ms, Amithuddar, Aymen0909, B2, Bnke0x0, Deivitto, Diana, Dinesh11G, Franfran, JC, JrNet, Jujic, KingNFT, KoKo, Mathieu, Metatron, Mukund, Olivierdem, PaludoX0, Pheonix, Picodes, RaymondFam, RedOneN, ReyAdmirado, Rolezn, Saintcode_, Satyam_Sharma, Shinchan, Tagir2003, Tomio, Waze, Yiko, __141345__, adriro, ajtra, aysha, ballx, beardofginger, bobirichman, brgltd, bulej93, catchup, catwhiskeys, cdahlheimer, ch0bu, chaduke, chrisdior4, cryptostellar5, cylzxje, d3e4, delfin454000, dharma09, djxploit, durianSausage, emrekocak, erictee, exolorkistis, fatherOfBlocks, gianganhnguyen, gogo, halden, hxzy, i_got_hacked, iepathos, karanctf, leosathya, lucacez, lukris02, lyncurion, m_Rassska, martin, mcwildy, mics, nicobevi, peanuts, peiw, rbserver, ret2basic, rotcivegaf, ryshaw, sakman, sakshamguruji, saneryee, sikorico, skyle, svskaushik, tnevler, vv7, w0Lfrum, zishansami
26.3525 USDC - $26.35
It's cheaper to store the length of the array inside a local variable and iterate over it.
Affected source code:
Using compound assignment operators for state variables (like State += X or State -= X ...) it's more expensive than using operator assignment (like State = State + X or State = State - X ...).
Proof of concept (without optimizations):
pragma solidity 0.8.15; contract TesterA { uint private _a; function testShort() public { _a += 1; } } contract TesterB { uint private _a; function testLong() public { _a = _a + 1; } }
Gas saving executing: 13 per entry
TesterA.testShort: 43507 TesterB.testLong: 43494
Affected source code:
An empty block or an empty method generally indicates a lack of logic that it’s unnecessary and should be eliminated, call a method that literally does nothing only consumes gas unnecessarily, if it is a virtual method which is expects it to be filled by the class that implements it, it is better to use abstract contracts with just the definition.
Sample code:
pragma solidity >=0.4.0 <0.7.0; abstract contract BaseContract { function totalSupply() public virtual returns (uint256); }
Reference:
Affected source code:
abi.encode instead of abi.encodePackedabi.encodePacked has a cost difference with respect to abi.encode depending on the types it uses. In the case of the reference shown below you can see that the type address is affected, so I have decided to do a test on it.
Also there is a discussion about removing abi.encodePacked from future versions of Solidity (ethereum/solidity#11593), so using abi.encode now will ensure compatibility in the future.
Reference:
https://github.com/ConnorBlockchain/Solidity-Encode-Gas-Comparison#address-comparison
Proof of concept (without optimizations):
pragma solidity ^0.8.12; contract TesterA { function getSaltEncode(address creator, uint256 nonce) public pure returns (bytes32) { return keccak256(abi.encode(creator, nonce)); } } contract TesterB { function getSaltEncodePacked(address creator, uint256 nonce) public pure returns (bytes32) { return keccak256(abi.encodePacked(creator, nonce)); } }
Gas saving executing: 188 per call
TesterA.getSaltEncode: 22774 TesterB.getSaltEncodePacked: 22962
It's recommended to apply the following changes:
function _getSalt(address creator, uint256 nonce) private pure returns (bytes32) { - return keccak256(abi.encodePacked(creator, nonce)); + return keccak256(abi.encode(creator, nonce)); }
Affected source code:
calldata instead of memorySome methods are declared as external but the arguments are defined as memory instead of as calldata.
By marking the function as external it is possible to use calldata in the arguments shown below and save significant gas.
Recommended change:
- function init(bytes memory initPayload) external override returns (bytes4) { ... } + function init(bytes calldata initPayload) external override returns (bytes4) { ... }
Affected source code:
++i costs less gas compared to i++ or i += 1++i costs less gas compared to i++ or i += 1 for unsigned integers, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.
i++ increments i and returns the initial value of i. Which means:
uint i = 1; i++; // == 1 but i == 2
But ++i returns the actual incremented value:
uint i = 1; ++i; // == 2 and i == 2 too, so no need for a temporary variable
In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2
I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--
Keep in mind that this change can only be made when we are not interested in the value returned by the operation, since the result is different, you only have to apply it when you only want to increase a counter.
Affected source code:
Several methods have been found in which it is possible to reduce the number of mathematical operations, the cases are detailed below.
Affected source code:
Use scientific notation rather than exponentiation:
10**18=> 1e18 : HolographOperator.sol:25610**10=> 1e10 : LayerZeroModule.sol:27410**10=> 1e10 : LayerZeroModule.sol:293Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.
Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.
Proof of concept (without optimizations):
pragma solidity 0.8.15; contract TesterA { function testShortRevert(bool path) public view { require(path, "test error"); } } contract TesterB { function testLongRevert(bool path) public view { require(path, "test big error message, more than 32 bytes"); } }
Gas saving executing: 18 per entry
TesterA.testShortRevert: 21886 TesterB.testLongRevert: 21904
Affected source code:
Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)
Source Custom Errors in Solidity:
Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.
Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).
Proof of concept (without optimizations):
pragma solidity 0.8.15; contract TesterA { function testRevert(bool path) public view { require(path, "test error"); } } contract TesterB { error MyError(string msg); function testError(bool path) public view { if(path) revert MyError("test error"); } }
Gas saving executing: 9 per entry
TesterA.testRevert: 21611 TesterB.testError: 21602
Affected source code:
Having a method that always returns the same value is not correct in terms of consumption, if you want to modify a value, and the method will perform a revert in case of failure, it is not necessary to return a true, since it will never be false. It is less expensive not to return anything, and it also eliminates the need to double-check the returned value by the caller.
Affected source code:
delete optimizationUse delete instead of set to default value (false or 0).
5 gas could be saved per entry in the following affected lines:
Affected source code:
bool to uint256 can save gasBecause each write operation requires an additional SLOAD to read the slot's contents, replace the bits occupied by the boolean, and then write back, booleans are more expensive than uint256 or any other type that uses a complete word. This cannot be turned off because it is the compiler's defense against pointer aliasing and contract upgrades.
Reference:
Also, this is applicable to integer types different from uint256 or int256.
Affected source code for booleans:
Affected source code for uint32:
If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.
Proof of concept (without optimizations):
pragma solidity 0.8.15; contract TesterA { function testInit() public view returns (uint) { uint a = 0; return a; } } contract TesterB { function testNoInit() public view returns (uint) { uint a; return a; } }
Gas saving executing: 8 per entry
TesterA.testInit: 21392 TesterB.testNoInit: 21384
Affected source code:
HolographERC20.onERC20ReceivedIs not required to check the _isContract line because it will be checked during the balanceOf call.
function onERC20Received( address account, address sender, uint256 amount, bytes calldata data ) public returns (bytes4) { - require(_isContract(account), "ERC20: operator not contract"); if (_isEventRegistered(HolographERC20Event.beforeOnERC20Received)) { require(SourceERC20().beforeOnERC20Received(account, sender, address(this), amount, data)); } try ERC20(account).balanceOf(address(this)) returns (uint256 balance) { require(balance >= amount, "ERC20: balance check failed"); } catch { revert("ERC20: failed getting balance"); } if (_isEventRegistered(HolographERC20Event.afterOnERC20Received)) { require(SourceERC20().afterOnERC20Received(account, sender, address(this), amount, data)); } return ERC20Receiver.onERC20Received.selector; }
Affected source code:
HolographERC721.onERC721ReceivedIs not required to check the _isContract line because it will be checked during the ownerOf call.
function onERC721Received( address _operator, address _from, uint256 _tokenId, bytes calldata _data ) external returns (bytes4) { - require(_isContract(_operator), "ERC721: operator not contract"); if (_isEventRegistered(HolographERC721Event.beforeOnERC721Received)) { require(SourceERC721().beforeOnERC721Received(_operator, _from, address(this), _tokenId, _data)); } try HolographERC721Interface(_operator).ownerOf(_tokenId) returns (address tokenOwner) { require(tokenOwner == address(this), "ERC721: contract not token owner"); } catch { revert("ERC721: token does not exist"); } if (_isEventRegistered(HolographERC721Event.afterOnERC721Received)) { require(SourceERC721().afterOnERC721Received(_operator, _from, address(this), _tokenId, _data)); } return ERC721TokenReceiver.onERC721Received.selector; }
Affected source code:
PA1D._validatePayoutRequestorIt's possible to avoid variables and conditions like this:
function _validatePayoutRequestor() private view { if (!isOwner()) { - bool matched; address payable[] memory addresses = _getPayoutAddresses(); address payable sender = payable(msg.sender); for (uint256 i = 0; i < addresses.length; i++) { if (addresses[i] == sender) { - matched = true; - break; + return; } } - require(matched, "PA1D: sender not authorized"); + revert("PA1D: sender not authorized"); } }
Affected source code:
The following methods are public, it's better to use private or internal:
HolographBridge.revertedBridgeOutRequest:function revertedBridgeOutRequest( address sender, uint32 toChain, address holographableContract, bytes calldata bridgeOutPayload - ) external returns (string memory revertReason) { + ) internal returns (string memory revertReason) {
Affected source code:
HolographERC721.unbondUtilityTokenIs not required to check the _isContract line because it will be checked during the unbondUtilityToken call.
function unbondUtilityToken(address operator, address recipient) external { /** * @dev validate that operator is currently bonded */ require(_bondedOperators[operator] != 0, "HOLOGRAPH: operator not bonded"); /** * @dev check if sender is not actual operator */ if (msg.sender != operator) { /** * @dev check if operator is a smart contract */ - require(_isContract(operator), "HOLOGRAPH: operator not contract"); /** * @dev check if smart contract is owned by sender */ require(Ownable(operator).isOwner(msg.sender), "HOLOGRAPH: sender not owner"); } ... }
Affected source code: