Platform: Code4rena
Start Date: 12/07/2023
Pot Size: $80,000 USDC
Total HM: 11
Participants: 47
Period: 9 days
Judge: berndartmueller
Total Solo HM: 1
Id: 260
League: ETH
Rank: 11/47
Findings: 3
Award: $1,117.45
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: immeas
Also found by: Bauchibred, DavidGiladi, Emmanuel, Jeiwan, MohammedRizwan, MrPotatoMagic, Rolezn, Sathish9098, T1MOH, Udsen, banpaleo5, hals, matrix_0wl, naman1778
43.3267 USDC - $43.33
_getTokenMintLimitKey,_getTokenTypeKey,_getTokenAddressKey,_getIsCommandExecutedKey
functions not prevent the duplicate symbol
valuesIf the functions _getTokenMintLimitKey
, _getTokenTypeKey
, _getTokenAddressKey
, and _getIsCommandExecutedKey
do not incorporate any uniqueness, and symbol
is the only varying parameter, then there will be hash collisions
for duplicate symbol
values.
FILE: 2023-07-axelar/contracts/cgp/AxelarGateway.sol function _getTokenMintLimitKey(string memory symbol) internal pure returns (bytes32) { return keccak256(abi.encodePacked(PREFIX_TOKEN_MINT_LIMIT, symbol)); } function _getTokenTypeKey(string memory symbol) internal pure returns (bytes32) { return keccak256(abi.encodePacked(PREFIX_TOKEN_TYPE, symbol)); } function _getTokenAddressKey(string memory symbol) internal pure returns (bytes32) { return keccak256(abi.encodePacked(PREFIX_TOKEN_ADDRESS, symbol)); } function _getIsCommandExecutedKey(bytes32 commandId) internal pure returns (bytes32) { return keccak256(abi.encodePacked(PREFIX_COMMAND_EXECUTED, commandId)); }
Unique nonce or timestamp in the abi.encodePacked
process to ensure that each call produces a unique hash
uint256 private nonce; function _getTokenMintLimitKey(string memory symbol) private returns (bytes32) { return keccak256(abi.encodePacked(PREFIX_TOKEN_MINT_LIMIT, symbol, nonce++)); }
setFlowLimit()
function Poses security risk in contractThe problem is the services paused by onlyOwner
. But FlowLimit
set by onlyOperator
this will lead to unintended consequences.
Allowing setFlowLimit()
to execute when the contract is paused can lead to unintended consequences and may compromise the expected behavior of the paused contract. Depending on how the contract utilizes the FlowLimit
, this can introduce potential security vulnerabilities or operational issues.
FILE: 2023-07-axelar/contracts/its/interchain-token-service/InterchainTokenService.sol function setFlowLimit(bytes32[] calldata tokenIds, uint256[] calldata flowLimits) external onlyOperator { uint256 length = tokenIds.length; if (length != flowLimits.length) revert LengthMismatch(); for (uint256 i; i < length; ++i) { ITokenManager tokenManager = ITokenManager(getValidTokenManagerAddress(tokenIds[i])); tokenManager.setFlowLimit(flowLimits[i]); } }
Incorporate the notPaused
modifier in the setFlowLimit()
function
OnlyOwner
allows malicious address as a trusted addresshe function allows the contract owner to add any address as a trusted address, regardless of whether the address is actually controlled by the interchain token service. This could allow an attacker to gain control of the contract by adding a malicious address as a trusted address.
FILE: 2023-07-axelar/contracts/its/remote-address-validator/RemoteAddressValidator.sol function addTrustedAddress(string memory chain, string memory addr) public onlyOwner { if (bytes(chain).length == 0) revert ZeroStringLength(); if (bytes(addr).length == 0) revert ZeroStringLength(); remoteAddressHashes[chain] = keccak256(bytes(_lowerCase(addr))); remoteAddresses[chain] = addr; emit TrustedAddressAdded(chain, addr); }
addTrustedAddress
function could be modified to require that the address to be added be signed by the interchain token service
. This would ensure that only trusted addresses
are added to the contract.
Two constants PREFIX_EXPRESS_RECEIVE_TOKEN
and PREFIX_EXPRESS_RECEIVE_TOKEN_WITH_DATA
are hardcoded. This means that anyone who knows the contract code will know what the prefixes are. This could make it easier for attackers to forge transactions that look like they were sent from the Axelar gateway.
codehash != 0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470
hardcoded value check is not appreciated . This will cause problem in future .
Possible Problems:
FILE: Breadcrumbs2023-07-axelar/contracts/cgp/AxelarGateway.sol 509: return codehash != bytes32(0) && codehash != 0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470;
FILE: 2023-07-axelar/contracts/its/utils/ExpressCallHandler.sol uint256 internal constant PREFIX_EXPRESS_RECEIVE_TOKEN = 0x67c7b41c1cb0375e36084c4ec399d005168e83425fa471b9224f6115af865619; // uint256(keccak256('prefix-express-give-token-with-data')); uint256 internal constant PREFIX_EXPRESS_RECEIVE_TOKEN_WITH_DATA = 0x3e607cc12a253b1d9f677a03d298ad869a90a8ba4bd0fb5739e7d79db7cdeaad;
It would be better to generate the prefixes dynamically and in lowercase. This would make the contract more secure and more flexible
sstore
instruction is a mutable instruction, which means that it can be overwritten by another transactionThe _setExpressReceiveToken
function uses the sstore
assembly instruction to store the value of the expressCaller
variable in the contract storage. The sstore
instruction is a mutable instruction, which means that it can be overwritten by another transaction. This means that if an attacker were to send a transaction that overwrites the value of the expressCaller
variable, they could effectively steal the tokens that were sent to the destination address
FILE: 2023-07-axelar/contracts/its/utils/ExpressCallHandler.sol if (prevExpressCaller != address(0)) revert AlreadyExpressCalled(); assembly { sstore(slot, expressCaller) }
_setExpressReceiveToken
function, the sstore
instruction should be used with the lock modifier. The lock modifier prevents the value of the expressCaller
variable from being overwritten by another transaction
IERC20.allowance()
function to check the allowance
instead of a custom mapping
Custom mappings consume storage on the Ethereum blockchain. If you have a large number of users or tokens, maintaining allowances in a mapping can lead to higher storage costs. Multiple concurrent transactions attempting to update allowances, there may be potential concurrency issues or race conditions.When using a custom mapping, you need to ensure that the allowances are synchronized and consistent between different parts of the contract
FILE: 2023-07-axelar/contracts/its/interchain-token/InterchainToken.sol 55: uint256 allowance_ = allowance[sender][address(tokenManager)];
Use IERC20.allowance()
function for handling token allowances.
The safeIncreaseAllowance()
and safeDecreaseAllowance()
functions are more secure than the approve()
function. The approve()
function allows an account to approve another account to transfer an unlimited amount of tokens on their behalf. This could be exploited by an attacker to transfer more tokens than the sender intended.
FILE: Breadcrumbs2023-07-axelar/contracts/its/interchain-token/InterchainToken.sol 61: _approve(sender, address(tokenManager), allowance_ + amount); 100: _approve(sender, address(tokenManager), allowance_ + amount);
safeIncreaseAllowance()
and safeDecreaseAllowance()
functions prevent this by ensuring that the amount of tokens that an account is approved to transfer cannot be increased or decreased by more than the current allowance.
Some tokens revert when transferring a zero value amount. Many ERC-20 and ERC-721 token contracts implement a safeguard that reverts transactions which attempt to transfer tokens with zero amount. This is because such transfers are often the result of programming errors. The OpenZeppelin library, a popular choice for implementing these standards, includes this safeguard. For token contract developers who want to avoid unintentional transfers with zero amount, it's good practice to include a condition that reverts the transaction if the amount is zero.
451: SafeTokenTransferFrom.safeTransferFrom(token, caller, destinationAddress, amount);
482: SafeTokenTransferFrom.safeTransferFrom(token, caller, destinationAddress, amount);
48: SafeTokenTransferFrom.safeTransferFrom(token, from, address(this), amount);
82: SafeTokenTransferFrom.safeTransferFrom(token, from, liquidityPool_, amount);
98: SafeTokenTransferFrom.safeTransferFrom(token, liquidityPool(), to, amount);
Make sure amount > 0
before calling transfer function
Some tokens (e.g. BNB) revert when approving a zero value amount (i.e. a call to approve(address, 0)). Integrators may need to add special cases to handle this logic if working with such a token.
61: _approve(sender, address(tokenManager), allowance_ + amount); 100: _approve(sender, address(tokenManager), allowance_ + amount);
registerCanonicalToken()
Not checking whether the tokenaddress
already managed by a token managerIf two different token managers are managing the same token address
, there is a risk
of conflicting behavior
. For example, one token manager might allow transfers while another token manager might disallow transfers. This could lead to errors or unexpected behavior for users.
FILE: 2023-07-axelar/contracts/its/interchain-token-service/InterchainTokenService.sol function registerCanonicalToken(address tokenAddress) external payable notPaused returns (bytes32 tokenId) { (, string memory tokenSymbol, ) = _validateToken(tokenAddress); if (gateway.tokenAddresses(tokenSymbol) == tokenAddress) revert GatewayToken(); tokenId = getCanonicalTokenId(tokenAddress); _deployTokenManager(tokenId, TokenManagerType.LOCK_UNLOCK, abi.encode(address(this).toBytes(), tokenAddress)); }
Add isTokenManaged()
function to check the token address existence
function registerCanonicalToken(address tokenAddress) external payable notPaused returns (bytes32 tokenId) { (, string memory tokenSymbol, ) = _validateToken(tokenAddress); if (gateway.tokenAddresses(tokenSymbol) == tokenAddress) revert GatewayToken(); + if (isTokenManaged(tokenAddress)) revert TokenAlreadyManaged(); tokenId = getCanonicalTokenId(tokenAddress); _deployTokenManager(tokenId, TokenManagerType.LOCK_UNLOCK, abi.encode(address(this).toBytes(), tokenAddress)); }
If the intention is for the Ether to be used, the function should call another function or emit something, otherwise it should revert (e.g. require(msg.sender == address(weth)))
FILE: 2023-07-axelar/contracts/cgp/governance/InterchainGovernance.sol 168: receive() external payable {}
_approve()
doesn’t check return valueif the approve() function fails, the caller will not be notified of the failure
61: _approve(sender, address(tokenManager), allowance_ + amount); 100: _approve(sender, address(tokenManager), allowance_ + amount);
Add the control to check _approve() status
receive()
function cannot be withdrawnThe receive()
function is used to handle incoming ether (native currency) transfers to a contract. However, when tokens (other than the native currency) are accidentally sent to the receive() function, they are effectively trapped within the contract, and there is no built-in mechanism
to withdraw
or recover them
.
FILE: 2023-07-axelar/contracts/cgp/governance/InterchainGovernance.sol 168: receive() external payable {}
Add Recovery mechanism to recover Eth sent accidently to the contract
executeProposal()
functionThere is a lack of access control in the executeProposal()
function of the Interchain Governance contract. Without access control, any external account can call this function and execute governance proposals, which may lead to unauthorized or malicious actions being taken
FILE: 2023-07-axelar/contracts/cgp/governance/InterchainGovernance.sol function executeProposal( address target, bytes calldata callData, uint256 nativeValue ) external payable { bytes32 proposalHash = keccak256(abi.encodePacked(target, callData, nativeValue)); _finalizeTimeLock(proposalHash); _call(target, callData, nativeValue); emit ProposalExecuted(proposalHash, target, callData, nativeValue, block.timestamp); }
Add modifiers like onlyOwner
to execute the proposals
sourceChain
and sourceAddress
string comparison is vulnerable to timing attacksIn the _execute
function, the contract uses string comparison to verify if the provided sourceChain
and sourceAddress
match the expected governance chain and address. This method is vulnerable to timing attacks, where an attacker could exploit the time taken for the comparison to determine partial information about the comparison result.
FILE: 2023-07-axelar/contracts/cgp/governance/InterchainGovernance.sol if (keccak256(bytes(sourceChain)) != governanceChainHash || keccak256(bytes(sourceAddress)) != governanceAddressHash) revert NotGovernance();
Use bytes32
for governanceChainHash
and governanceAddressHash
, and use == for direct comparison
The sendProposal function in the InterchainProposalSender contract can be susceptible to replay attacks without a nonce mechanism because it allows users to broadcast the same proposal multiple times.
leading to the following issues:
function sendProposals(InterchainCalls.InterchainCall[] calldata interchainCalls) external payable override { // revert if the sum of given fees are not equal to the msg.value revertIfInvalidFee(interchainCalls); for (uint256 i = 0; i < interchainCalls.length; ) { _sendProposal(interchainCalls[i]); unchecked { ++i; } } }
Add nonce mechanism to sendProposal
function
RemoteAddressValidator.sol
is inherited ``upgradable` but not actually ungradable contractThe absence of an initialize
function suggests that the contract is not intended to be upgradable using a proxy contract. Instead, it follows a traditional deployment approach, where the constructor is used to set up the initial state of the contract. Once deployed, the state and behavior of the contract are immutable, and any changes or updates would require deploying a new instance of the contract.
FILE: 2023-07-axelar/contracts/its/remote-address-validator/RemoteAddressValidator.sol 12: contract RemoteAddressValidator is IRemoteAddressValidator, Upgradable {
Index event fields make the field more quickly accessible to off-chain.
Each event should use three indexed fields if there are three or more fields.
#0 - c4-judge
2023-09-08T11:23:34Z
berndartmueller marked the issue as grade-b
🌟 Selected for report: Sathish9098
Also found by: 0x11singh99, 0xAnah, 0xn006e7, Arz, DavidGiladi, K42, Raihan, ReyAdmirado, Rolezn, SAQ, SM3_SS, SY_S, Walter, dharma09, flutter_developer, hunter_w3b, matrix_0wl, naman1778, petrichor, ybansal2403
246.6663 USDC - $246.67
GAS COUNT | Issues | Instances | Gas Saved |
---|---|---|---|
[G-1] | Multiple address mappings can be combined into a single mapping of an address to a struct, where appropriate | 1 | 21018 |
[G-2] | Using calldata instead of memory for read-only arguments in external functions saves gas | 8 | 2820 |
[G-3] | Avoiding the overhead of bool storage | 6 | 100600 |
[G-4] | Avoid contract existence checks by using low level calls | 37 | 3700 |
[G-5] | Use calldata pointer Saves more gas than memory pointer | 2 | 600 GAS (Per Iteration) |
[G-6] | IF’s/require() statements that check input arguments should be at the top of the function | 2 | 300 |
[G-7] | Functions guaranteed to revert when called by normal users can be marked payable | 11 | 231 |
[G-8] | Optimize names to save gas | 27 | - |
[G-9] | Default value initialization | 4 | 80 |
[G-10] | Use constants instead of type(uintX).max | 7 | 91 |
[G-11] | Splitting require()/if() statements that use && saves gas | 4 | 52 |
[G-12] | Caching global variables is more expensive than using the actual variable (use msg.sender instead of caching it) | 10 | - |
We can combine multiple mappings below into structs. We can then pack the structs by modifying the uint type for the values. This will result in cheaper storage reads since multiple mappings are accessed in functions and those values are now occupying the same storage slot, meaning the slot will become warm after the first SLOAD
. In addition, when writing to and reading from the struct values we will avoid a Gsset (20000 gas)
and Gcoldsload (2100 gas)
, since multiple struct values are now occupying the same slot.
RemoteAddressValidator.sol
: struct can be used for remoteAddressHashes
,remoteAddresses
since they are using same string
as key. Also both mapping always used together in a same functions like addTrustedAddress()
, removeTrustedAddress()
So struct
is more efficient than mapping
. As per gas tests this will save 21018 GAS
FILE: 2023-07-axelar/contracts/its/remote-address-validator/RemoteAddressValidator.sol - 15: mapping(string => bytes32) public remoteAddressHashes; - 16: mapping(string => string) public remoteAddresses; + struct RemoteAddressData { + bytes32 addressHash; + string addressString; + } + mapping(string => RemoteAddressData) public remoteAddressData;
FILE: Breadcrumbs2023-07-axelar/contracts/its/remote-address-validator/RemoteAddressValidator.sol 15: mapping(string => bytes32) public remoteAddressHashes; 16: mapping(string => string) public remoteAddresses;
Saves 2820 GAS
, 8 Instances
When a function with a memory array is called externally, the abi.decode()
step has to use a for-loop to copy each index of the calldata
to the memory
index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length)
. Using calldata
directly, obliviates the need for such a loop in the contract code and runtime execution. Note that even if an interface defines a function as having memory arguments, it’s still valid for implementation contracts to use calldata
arguments instead.
If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it’s still more gass-efficient to use calldata
when the external
function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one.
InterchainTokenService.sol
: distributor
,operator
can be calldata
instead of memory
: Saves 564 GAS
FILE: 2023-07-axelar/contracts/its/interchain-token-service/InterchainTokenService.sol 417: function deployAndRegisterRemoteStandardizedToken( 418: bytes32 salt, 419: string calldata name, 420: string calldata symbol, 421: uint8 decimals, - 422: bytes memory distributor, + 422: bytes calldata distributor, - 423: bytes memory operator, + 423: bytes calldata operator, 424: string calldata destinationChain, 425: uint256 gasValue 426: ) external payable notPaused {
InterchainTokenService.sol
: sourceChain
,sourceAddress
,destinationAddress
can be calldata
instead of memory
: Saves 846 GAS
FILE: 2023-07-axelar/contracts/its/interchain-token-service/InterchainTokenService.sol 467: function expressReceiveTokenWithData( 468: bytes32 tokenId, - 469: string memory sourceChain, + 469: string calldata sourceChain, - 470: bytes memory sourceAddress, + 470: bytes calldata sourceAddress, 471: address destinationAddress, 472: uint256 amount, 473: bytes calldata data, 474: bytes32 commandId 475: ) external { - 506: bytes memory destinationAddress, + 506: bytes calldata destinationAddress,
MultisigBase.sol
: newAccounts
can be calldata
instead of memory
: Saves 282 GAS
FILE: 2023-07-axelar/contracts/cgp/auth/MultisigBase.sol - 142: function rotateSigners(address[] memory newAccounts, uint256 newThreshold) external virtual onlySigners { + 142: function rotateSigners(address[] calldata newAccounts, uint256 newThreshold) external virtual onlySigners { 143: _rotateSigners(newAccounts, newThreshold); 144: }
InterchainProposalSender.sol
: destinationChain
,destinationContract
can be calldata
instead of memory
: Saves 564 GAS
FILE: 2023-07-axelar/contracts/interchain-governance-executor/InterchainProposalSender.sol 80: function sendProposal( - 81: string memory destinationChain, + 81: string calldata destinationChain, - 82: string memory destinationContract, + 82: string calldata destinationContract, 83: InterchainCalls.Call[] calldata calls 84: ) external payable override {
ConstAddressDeployer.sol
: bytecode
,bytecode
can be calldata
instead of memory
: Saves 564 GAS
FILE: Breadcrumbs2023-07-axelar/contracts/gmp-sdk/deploy/ConstAddressDeployer.sol - 24: function deploy(bytes memory bytecode, bytes32 salt) external returns (address deployedAddress_) { + 24: function deploy(bytes calldata bytecode, bytes32 salt) external returns (address deployedAddress_) { 42: function deployAndInit( - 43: bytes memory bytecode, + 43: bytes calldata bytecode, 44: bytes32 salt, 45: bytes calldata init 46: ) external returns (address deployedAddress_) {
Saves 120600 GAS
, 6 Instances
// 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 to avoid a Gwarmaccess
(100 gas)
for the extra SLOAD
, and to avoid Gsset
(20000 gas)
when changing from false to true, after having been true in the past
FILE: 2023-07-axelar/contracts/cgp/auth/MultisigBase.sol 15: mapping(address => bool) hasVoted; 21: mapping(address => bool) isSigner;
FILE: 2023-07-axelar/contracts/cgp/governance/AxelarServiceGovernance.sol 22: mapping(bytes32 => bool) public multisigApprovals;
FILE: 2023-07-axelar/contracts/interchain-governance-executor/InterchainProposalExecutor.sol 24: mapping(string => mapping(address => bool)) public whitelistedCallers; 27: mapping(string => mapping(address => bool)) public whitelistedSenders;
FILE: 2023-07-axelar/contracts/its/remote-address-validator/RemoteAddressValidator.sol 19: mapping(string => bool) public supportedByGateway;
Saves 3700 GAS
, 37 Instances
Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE
(100 gas), to check for contract existence for external function calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value. Similar behavior can be achieved in earlier versions by using low-level calls, since low level calls never check for contract existence
FILE: Breadcrumbs2023-07-axelar/contracts/its/interchain-token-service/InterchainTokenService.sol 102: deployer = ITokenManagerDeployer(tokenManagerDeployer_).deployer(); 162: tokenManagerAddress = deployer.deployedAddress(address(this), tokenId); 172: if (ITokenManagerProxy(tokenManagerAddress).tokenId() != tokenId) revert TokenManagerDoesNotExist(tokenId); 182: tokenAddress = ITokenManager(tokenManagerAddress).tokenAddress(); 193: tokenAddress = deployer.deployedAddress(address(this), tokenId); 277: flowLimit = tokenManager.getFlowLimit(); 287: flowOutAmount = tokenManager.getFlowOutAmount(); 297: flowInAmount = tokenManager.getFlowInAmount(); 311: if (gateway.tokenAddresses(tokenSymbol) == tokenAddress) revert GatewayToken(); 331: tokenAddress = ITokenManager(tokenAddress).tokenAddress(); 445: if (gateway.isCommandExecuted(commandId)) revert AlreadyExecuted(commandId); 476: if (gateway.isCommandExecuted(commandId)) revert AlreadyExecuted(commandId); 480: IERC20 token = IERC20(tokenManager.tokenAddress()); 539: tokenManager.setFlowLimit(flowLimits[i]); 566: if (ITokenManager(implementation).implementationType() != uint256(tokenManagerType)) revert InvalidTokenManagerImplementation(); 610: amount = tokenManager.giveToken(destinationAddress, amount); 653: amount = tokenManager.giveToken(expressCaller, amount); 657: amount = tokenManager.giveToken(destinationAddress, amount); 658: IInterchainTokenExpressExecutable(destinationAddress).executeWithInterchainToken(sourceChain, sourceAddress, data, tokenId, amount); 713: string memory destinationAddress = remoteAddressValidator.getRemoteAddress(destinationChain); 723: gateway.callContract(destinationChain, destinationAddress, payload); 735: name = token.name(); 736: symbol = token.symbol(); 737: decimals = token.decimals(); 888: IInterchainTokenExpressExecutable(destinationAddress).expressExecuteWithInterchainToken( sourceChain, sourceAddress, data, tokenId, amount );
FILE: Breadcrumbs2023-07-axelar/contracts/cgp/AxelarGateway.sol 287: if (AxelarGateway(newImplementation).contractId() != contractId()) revert InvalidImplementation(); 317: IAxelarAuth(AUTH_MODULE).transferOperatorship(newOperatorsData); 329: bool allowOperatorshipTransfer = IAxelarAuth(AUTH_MODULE).validateProof(messageHash, proof); 449: depositHandler.destroy(address(this) 496: IAxelarAuth(AUTH_MODULE).transferOperatorship(newOperatorsData); 524: IERC20(tokenAddress).safeTransfer(account, amount); 526: IBurnableMintableCappedERC20(tokenAddress).mint(account, amount); 543: IERC20(tokenAddress).safeTransferFrom(sender, address(this), amount); 545: IERC20(tokenAddress).safeCall(abi.encodeWithSelector(IBurnableMintableCappedERC20.burnFrom.selector, sender, amount)); 547: IERC20(tokenAddress).safeTransferFrom(sender, IBurnableMintableCappedERC20(tokenAddress).depositAddress(bytes32(0)), amount);
FILE: 2023-07-axelar/contracts/interchain-governance-executor/InterchainProposalSender.sol 101: gateway.callContract(interchainCall.destinationChain, interchainCall.destinationContract, payload);
Calldata
pointer Saves more gas than memory
pointerSaves 600 GAS
, per Loop Iterations
Calling calldata
instead of memory
in the loop you have shown will save gas. This is because calldata
is a read-only data structure, which means that it does not have to be copied into memory each time it is accessed.
InterchainProposalExecutor.sol
: Use ```calldatainstead of
memory: Saves
250-300 GAS`` Per iterationcall
value is not changed any where inside the loop. So calldata
is more efficient than memory
to save gas
FILE: Breadcrumbs2023-07-axelar/contracts/interchain-governance-executor/InterchainProposalExecutor.sol 74: for (uint256 i = 0; i < calls.length; i++) { - 75: InterchainCalls.Call memory call = calls[i]; + 75: InterchainCalls.Call calldata call = calls[i]; 76: (bool success, bytes memory result) = call.target.call{ value: call.value }(call.callData); 77: 78: if (!success) { 79: _onTargetExecutionFailed(call, result); 80: } else { 81: _onTargetExecuted(call, result); 82: }
AxelarGateway.sol
: Use ```calldatainstead of
memory: Saves
250-300 GAS`` Per iterationsymbol
value is not changed any where inside the loop. So calldata
is more efficient than memory
to save gas
FILE: 2023-07-axelar/contracts/cgp/AxelarGateway.sol 270: for (uint256 i; i < length; ++i) { - 271: string memory symbol = symbols[i]; + 271: string calldata symbol = symbols[i]; 272: uint256 limit = limits[i]; 273: 274: if (tokenAddresses(symbol) == address(0)) revert TokenDoesNotExist(symbol); 275: 276: _setTokenMintLimit(symbol, limit); 277: }
Saves 300 GAS
, 2 Instance
Checks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a Gcoldsload (2100 gas) in a function that may ultimately revert in the unhappy case.
200- 300 GAS
FILE: 2023-07-axelar/contracts/cgp/auth/MultisigBase.sol 169: address account = newAccounts[i]; 170: 171: // Check that the account wasn't already set as a signer for this epoch. + 273: if (account == address(0)) revert InvalidSigners(); - 172: if (signers.isSigner[account]) revert DuplicateSigner(account); - 273: if (account == address(0)) revert InvalidSigners(); + 172: if (signers.isSigner[account]) revert DuplicateSigner(account);
limit
to avoid unnecessary variable creation. After variable creation if any fails its waste of gasFILE: 2023-07-axelar/contracts/cgp/AxelarGateway.sol 271: string memory symbol = symbols[i]; + 274: if (tokenAddresses(symbol) == address(0)) revert TokenDoesNotExist(symbol); 272: uint256 limit = limits[i]; 273: - 274: if (tokenAddresses(symbol) == address(0)) revert TokenDoesNotExist(symbol);
payable
Saves 231 GAS
, 11 Instance
If 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
92: function setWhitelistedProposalCaller( 93: string calldata sourceChain, 94: address sourceCaller, 95: bool whitelisted 96: ) external override onlyOwner
107: function setWhitelistedProposalSender( 108: string calldata sourceChain, 109: address sourceSender, 110: bool whitelisted 111: ) external override onlyOwner
52: function upgrade( 53: address newImplementation, 54: bytes32 newImplementationCodeHash, 55: bytes calldata params 56: ) external override onlyOwner
547: function setPaused(bool paused) external onlyOwner
83: function addTrustedAddress(string memory chain, string memory addr) public onlyOwner
95: function removeTrustedAddress(string calldata chain) external onlyOwner
106: function addGatewaySupportedChains(string[] calldata chainNames) external onlyOwner
119: function removeGatewaySupportedChains(string[] calldata chainNames) external onlyOwner
https://github.com/code-423n4/2023-07-axelar/tree/main/contracts/its/interchain-token-service/InterchainTokenService.sol#L343-L345 ```solidity 343: function deployCustomTokenManager( 344: bytes32 salt, 345: TokenManagerType tokenManagerType, 346: bytes memory params 347: ) public payable notPaused returns (bytes32 tokenId)
365: function deployRemoteCustomTokenManager( 366: bytes32 salt, 367: string calldata destinationChain, 368: TokenManagerType tokenManagerType, 369: bytes calldata params, 370: uint256 gasValue 371: ) external payable notPaused returns (bytes32 tokenId)
502: function transmitSendToken( 503: bytes32 tokenId, 504: address sourceAddress, 505: string calldata destinationChain, 506: bytes memory destinationAddress, 507: uint256 amount, 508: bytes calldata metadata 509: ) external payable onlyTokenManager(tokenId) notPaused
Saves 27 Instances
public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted
///@audit getProposalEta(),executeProposal(), FILE: 2023-07-axelar/contracts/cgp/governance/InterchainGovernance.sol ///@audit executeMultisigProposal(), FILE: 2023-07-axelar/contracts/cgp/governance/AxelarServiceGovernance.sol ///@audit getChainName(),getTokenManagerAddress(),getValidTokenManagerAddress(),getTokenAddress(),getStandardizedTokenAddress(),getCanonicalTokenId(),getCustomTokenId(),getImplementation(),getParamsLockUnlock(),getParamsMintBurn(),getParamsLiquidityPool(),getFlowLimit(),getFlowOutAmount(),getFlowInAmount(),registerCanonicalToken(),deployRemoteCanonicalToken(),deployCustomTokenManager(),deployRemoteCustomTokenManager(),deployAndRegisterStandardizedToken(),deployAndRegisterRemoteStandardizedToken(),expressReceiveToken(),expressReceiveTokenWithData(),transmitSendToken(),setFlowLimit(), FILE: 2023-07-axelar/contracts/its/interchain-token-service/InterchainTokenService.sol
Saves 80 GAS
,4 Instance
If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type)
.
Explicitly initializing it with its default value is an anti-pattern and wastes gas.
Saves 15-20
GAS per instance
FILE: 2023-07-axelar/contracts/interchain-governance-executor/InterchainProposalSender.sol - 63: for (uint256 i = 0; i < interchainCalls.length; ) { + 63: for (uint256 i; i < interchainCalls.length; ) { - 105: uint256 totalGas = 0; + 105: uint256 totalGas ; - 106: for (uint256 i = 0; i < interchainCalls.length; ) { + 106: for (uint256 i ; i < interchainCalls.length; ) {
FILE: 2023-07-axelar/contracts/interchain-governance-executor/InterchainProposalExecutor.sol - 74: for (uint256 i = 0; i < calls.length; i++) { + 74: for (uint256 i ; i < calls.length; i++) {
Saves 91 GAS
,7 Instance
Using constants instead of type(uintX).max saves gas in Solidity. This is because the type(uintX).max function has to dynamically calculate the maximum value of a uint256, which can be expensive in terms of gas. Constants, on the other hand, are stored in the bytecode of your contract, so they do not have to be recalculated every time you need them. Saves 13 GAS
FILE: 2023-07-axelar/contracts/its/interchain-token/InterchainToken.sol 56: if (allowance_ != type(uint256).max) { 57: if (allowance_ > type(uint256).max - amount) { 58: allowance_ = type(uint256).max - amount; 88: if (_allowance != type(uint256).max) { 95: if (allowance_ != type(uint256).max) { 96: if (allowance_ > type(uint256).max - amount) { 97: allowance_ = type(uint256).max - amount;
Saves 52 GAS
,4 Instance
See 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 by 13 gas
FILE: 2023-07-axelar/contracts/its/remote-address-validator/RemoteAddressValidator.sol 58: if ((b >= 65) && (b <= 70)) bytes(s)[i] = bytes1(b + uint8(32));
FILE: 2023-07-axelar/contracts/cgp/AxelarGateway.sol 88: if (msg.sender != getAddress(KEY_MINT_LIMITER) && msg.sender != getAddress(KEY_GOVERNANCE)) revert NotMintLimiter(); 446: if (!success || (returnData.length != uint256(0) && !abi.decode(returnData, (bool)))) revert BurnFailed(symbol); 635: if (limit > 0 && amount > limit) revert ExceedMintLimit(symbol);
The msg.sender
variable is a special variable that always refers to the address of the sender of the current transaction. This variable is not stored in memory, so it is much cheaper to use than a cached global variable.
FILE: Breadcrumbs2023-07-axelar/contracts/its/interchain-token-service/InterchainTokenService.sol - 348: address deployer_ = msg.sender; - 349: tokenId = getCustomTokenId(deployer_, salt); + 349: tokenId = getCustomTokenId(msg.sender, salt); 350: _deployTokenManager(tokenId, tokenManagerType, params); - 351: emit CustomTokenIdClaimed(tokenId, deployer_, salt); + 351: emit CustomTokenIdClaimed(tokenId, msg.sender, salt); - address deployer_ = msg.sender; - tokenId = getCustomTokenId(deployer_, salt); + tokenId = getCustomTokenId(msg.sender, salt); _deployRemoteTokenManager(tokenId, destinationChain, gasValue, tokenManagerType, params); - emit CustomTokenIdClaimed(tokenId, deployer_, salt); + emit CustomTokenIdClaimed(tokenId, msg.sender, salt); - address caller = msg.sender; ITokenManager tokenManager = ITokenManager(getValidTokenManagerAddress(tokenId)); IERC20 token = IERC20(tokenManager.tokenAddress()); - SafeTokenTransferFrom.safeTransferFrom(token, caller, destinationAddress, amount); + SafeTokenTransferFrom.safeTransferFrom(token, msg.sender, destinationAddress, amount); - _setExpressReceiveToken(tokenId, destinationAddress, amount, commandId, caller); + _setExpressReceiveToken(tokenId, destinationAddress, amount, commandId, msg.sender); - address caller = msg.sender; ITokenManager tokenManager = ITokenManager(getValidTokenManagerAddress(tokenId)); IERC20 token = IERC20(tokenManager.tokenAddress()); - SafeTokenTransferFrom.safeTransferFrom(token, caller, destinationAddress, amount); + SafeTokenTransferFrom.safeTransferFrom(token, msg.sender, destinationAddress, amount); _expressExecuteWithInterchainTokenToken(tokenId, destinationAddress, sourceChain, sourceAddress, data, amount); - _setExpressReceiveTokenWithData(tokenId, sourceChain, sourceAddress, destinationAddress, amount, data, commandId, caller); + _setExpressReceiveTokenWithData(tokenId, sourceChain, sourceAddress, destinationAddress, amount, data, commandId, msg.sender);
FILE: Breadcrumbs2023-07-axelar/contracts/its/interchain-token/InterchainToken.sol - address sender = msg.sender; ITokenManager tokenManager = getTokenManager(); /** * @dev if you know the value of `tokenManagerRequiresApproval()` you can just skip the if statement and just do nothing or _approve. */ if (tokenManagerRequiresApproval()) { - uint256 allowance_ = allowance[sender][address(tokenManager)];- + uint256 allowance_ = allowance[msg.sender][address(tokenManager)]; if (allowance_ != type(uint256).max) { if (allowance_ > type(uint256).max - amount) { allowance_ = type(uint256).max - amount; } - _approve(sender, address(tokenManager), allowance_ + amount); + _approve(msg.sender, address(tokenManager), allowance_ + amount); }
#0 - c4-pre-sort
2023-07-29T01:15:02Z
0xSorryNotSorry marked the issue as high quality report
#1 - c4-judge
2023-09-04T19:34:18Z
berndartmueller marked the issue as grade-a
#2 - c4-judge
2023-09-04T19:42:14Z
berndartmueller marked the issue as selected for report
🌟 Selected for report: pcarranzav
Also found by: Jeiwan, K42, MrPotatoMagic, Sathish9098, libratus, niloy
List | Head | Details |
---|---|---|
1 | Overview of Axelar Network Protocol | overview of the key components and features of Axelarb Network |
2 | My Thoughts | My own thoughts about future of the this protocol |
3 | Audit approach | Process and steps i followed |
4 | Learnings | Learnings from this protocol |
5 | Possible Systemic Risks | The possible systemic risks based on my analysis |
6 | Code Commentary | Suggestions for existing code base |
7 | Centralization risks | Concerns associated with centralized systems |
8 | Gas Optimizations | Details about my gas optimizations findings and gas savings |
9 | Risks | Possible risks |
10 | Non-functional aspects | general suggestions |
11 | Time spent on analysis | The Over all time spend for this reports |
Axelar is a decentralized interoperability network connecting all blockchains, assets and apps through a universal set of protocols and APIs.
Axelar could change the future in following areas
Decentralized finance (DeFi)
: Axelar could make it easier for users to access DeFi applications on different chains. This could help to increase the liquidity of DeFi markets and could also make DeFi applications more accessible to users.
NFTs
: Axelar could make it easier for users to trade NFTs between different chains. This could help to increase the liquidity of NFT markets and could also make NFTs more accessible to users.
Gaming
: Axelar could make it easier for users to play games across different chains. This could help to increase the reach of blockchain games and could also make games more immersive for users.
I followed below steps while analyzing and auditing the code base.
Analyzed the over all codebase one iterations very fast
Study of documentation to understand each contract purpose, its functionality, how it is connected with other contracts, etc.
Then i read old audits and already known findings. Then go through the bot races findings
Then setup my testing environment things. Run the tests to checks all test passed.
Finally, I started with the auditing the code base in depth way I started understanding line by line code and took the necessary notes to ask some questions to sponsors.
The first stage of the audit
During the initial stage of the audit for Axelar Network, the primary focus was on analyzing gas usage and quality assurance (QA) aspects. This phase of the audit aimed to ensure the efficiency of gas consumption and verify the robustness of the platform.
The second stage of the audit
In the second stage of the audit for Axelar Network, the focus shifted towards understanding the protocol usage in more detail. This involved identifying and analyzing the important contracts and functions within the system. By examining these key components, the audit aimed to gain a comprehensive understanding of the protocol's functionality and potential risks.
The third stage of the audit
During the third stage of the audit for Axelar Network, the focus was on thoroughly examining and marking any doubtful or vulnerable areas within the protocol. This stage involved conducting comprehensive vulnerability assessments and identifying potential weaknesses in the system. Found 60-70
vulnerable
and weakness
code parts all marked with @Audit tags
.
The fourth stage of the audit
During the fourth stage of the audit for Axelar Network, a comprehensive analysis and testing of the previously identified doubtful and vulnerable areas were conducted. This stage involved diving deeper into these areas, performing in-depth examinations, and subjecting them to rigorous testing, including fuzzing with various inputs. Finally concluded findings after all research's and tests. Then i reported C4 with proper formats
Axelar's architecture
: The Axelar network is composed of three main components: the relayers, the validators, and the gateway smart contracts. The relayers are responsible for transferring messages between chains, the validators are responsible for confirming events and commands on different chains, and the gateway smart contracts are responsible for managing the transfer of tokens and data between chains.
Axelar's security model
: Axelar's security model is based on a combination of decentralized validators and secure smart contracts. The decentralized validators are responsible for confirming events and commands on different chains, and the secure smart contracts are responsible for managing the transfer of tokens and data between chains.
Axelar's roadmap
: The Axelar team has a roadmap for the development of the network. The roadmap includes plans to add support for more chains, to improve the security of the network, and to make the network more user-friendly.
Relayer attacks
: A relayer could potentially attack the Axelar network by submitting malicious commands to the gateway smart contracts. This could result in the theft of tokens or the loss of data.
Double-spend attacks
: A user could potentially double-spend tokens by sending them to two different chains. This could be done by sending the tokens to one chain and then submitting a malicious command to the gateway smart contracts to cancel the transaction on the other chain.
Sybil attack
: A malicious actor could potentially create a large number of fake validators in order to gain control of the Axelar network. This could allow the malicious actor to approve malicious commands or to prevent legitimate commands from being processed.
Buggy smart contracts
: The Axelar gateway smart contracts could contain bugs that could be exploited by attackers. This could result in the theft of tokens or the loss of data.
Events
is missing indexed
fields
Index event fields make the field more quickly accessible to off-chain.Each event should use three indexed fields if there are three or more fields.
Shadowing variables can be confusing in contracts, as it can be difficult to track which variable is being referenced at any given time. This can lead to errors and unexpected behavior.
operator
shadowed varibale used in InterchainTokenService.sol
. The same varibale name used in Operator.sol
gateway
shadowed varibale used in InterchainGovernance.sol
. The same varibale name used in AxelarExecutable .sol
as IAxelarGateway
instancegateway, governanceChain, governanceAddress
used in AxelarServiceGovernance.sol
Use latest version of solidity. Use atleast 0.8.17
Don't use bool . Use uint(1)
and uint(2)
for true,false
Use abi.encode
instead of abi.encodePacked
when ever possible to avoid hash collisions
Use notPaused
Modifier in important functions
Consider using a struct to represent trusted addresses instead of separate arrays for chain names and addresses. This can enhance code readability and make it easier to manage trusted addresses
Explicitly specify the visibility modifiers for functions and state variables, such as public, external, internal, or private, based on their intended use
Consider using bytes32 for governanceChain and governanceAddress if they are fixed-size strings to save gas costs
Add informative error messages in require statements to provide clear feedback on revert reasons
Add safeguards to reentrnacy attacks
Use enum values directly instead of converting commandId to an IAxelarGateway.Command enum in the execute and executeWithToken functions, use the enum values directly to simplify the code.
Add validations in MultisigBase.sol
the constructor to ensure that the provided list of signers is not empty and that the threshold is not zero
Move the voting logic out of the onlySigners modifier and into a separate function, which can be called explicitly when a signer wants to vote on a specific topic. This separation improves code readability and allows for better control over when a vote is cast
Some important state changes might not be adequately captured by events. Consider emitting events for significant state changes to improve transparency and allow external systems to track contract activity
Add detailed comments explaining the purpose and functionality of each function. This can significantly improve code readability and ease maintenance
Ensure consistent naming conventions throughout the codebase to enhance clarity and reduce confusion
Consider breaking the contract into smaller, modular components. This can make the codebase more manageable and easier to maintain
A single point of failure is not acceptable for this project Centrality risk is high in the project, the role of onlyOwner
detailed below has very critical and important powers
Project and funds may be compromised by a malicious or stolen private key onlyOwner
msg.sender
92: function setWhitelistedProposalCaller( 93: string calldata sourceChain, 94: address sourceCaller, 95: bool whitelisted 96: ) external override onlyOwner
107: function setWhitelistedProposalSender( 108: string calldata sourceChain, 109: address sourceSender, 110: bool whitelisted 111: ) external override onlyOwner
52: function upgrade( 53: address newImplementation, 54: bytes32 newImplementationCodeHash, 55: bytes calldata params 56: ) external override onlyOwner
547: function setPaused(bool paused) external onlyOwner
83: function addTrustedAddress(string memory chain, string memory addr) public onlyOwner
95: function removeTrustedAddress(string calldata chain) external onlyOwner
106: function addGatewaySupportedChains(string[] calldata chainNames) external onlyOwner
119: function removeGatewaySupportedChains(string[] calldata chainNames) external onlyOwner
In the context of optimizing gas consumption, several improvements have been identified within the codebase of the Axelar protocol
. One of the main optimizations involves combining multiple address mappings into a single mapping of an address to a struct
where appropriate. By packing the structs and modifying the uint type for the values, storage
reads become more cost-efficient
, resulting in substantial gas savings
. Additionally, using the calldata
keyword instead of memory
for read-only arguments in external functions reduces gas usage significantly, as it avoids unnecessary data copying
.
Another gas optimization relates to avoiding the overhead
of using bool storage
, which incurs extra gas costs due to read and write operations. By replacing bool variables with appropriate uint values (e.g., 1 and 2) to represent true
and false
, the contract can avoid additional SLOAD
operations, leading to substantial gas savings across multiple instances.
Furthermore, using low-level calls
instead of external function calls can also save gas by eliminating contract existence checks
. This is particularly useful for functions that are expected to revert when called by regular users since it avoids unnecessary opcode execution
.
To further reduce gas consumption, the codebase can be updated to use calldata
pointers instead of memory
pointers, which offer cost savings in loop iterations. Additionally, moving IF
and require()
statements that check input arguments to the top of functions can avoid unnecessary computation
and potential gas waste
.
Marking functions guaranteed to revert when called by normal users as payable
is another optimization to reduce gas costs, as it avoids the extra gas spent on checking whether a payment was provided.
To enhance the readability and efficiency of the code, optimizing names
for functions and variables can lead to gas savings by reducing method IDs and method call costs.
Finally, by eliminating unnecessary default value initialization
, such as explicitly setting variables to their default values, gas savings can be achieved by allowing variables to use their default values automatically.
Overall, implementing these gas optimizations can significantly enhance the efficiency and cost-effectiveness of the Axelar protocol
, making it more robust and scalable for its users
The contract uses strings for governanceChain
and governanceAddress
. While it's possible to use strings for these purposes, it's generally better to use bytes32 for fixed-size strings to save gas costs and avoid potential security vulnerabilities. If the governance chain and address are known to have fixed lengths, consider using bytes32 instead.
contract emits events such as ProposalExecuted
, ProposalScheduled
, and ProposalCancelled
, there are no comments or documentation explaining what these events signify and the expected format of the emitted data.
Lack of access control in executeProposal()
function
The target
address could be an invalid or malicious contract address, leading to unexpected behavior or potential vulnerabilities. Ensure that you add appropriate input validation to check the validity of the target
address and other inputs.
The rotateSigners function is used to both rotate the signers and set the initial signers. It might be confusing to use the same function for both purposes. Consider splitting the functionality into two separate functions, one for the initial setup and another for rotating the signers
The getSignerVotesCount function iterates through all signers to count the votes for a topic. This operation has a linear complexity (O(n)), where n is the number of signers.
The onlySigners modifier modifies contract state by updating the vote count and setting the hasVoted mapping. There could be potential reentrancy vulnerabilities if any function called within the modifier allows external contract calls before the state modifications are completed
The contract uses keccak256(msg.data) to generate the vote topic hash. Using msg.data directly can lead to inconsistent hash generation due to differences in ABI encoding
The contract only provides a single signer threshold for executing operations. In more complex multisignature contracts, it might be beneficial to have different levels of signers with different threshold requirements for certain types of transactions
The contract does not perform any signature verification to ensure that the transactions are signed correctly by the required number of signers. Without proper signature validation, the security of the multisignature contract may be compromised
Some functions are marked as onlySelf, which means they can only be called by the contract itself. Make sure that these functions are intended for internal use only and cannot be called by unauthorized parties
Limit the use of delegatecall to only trusted and thoroughly audited contracts. If possible, avoid using delegatecall altogether as it can introduce complex security risks
return codehash != bytes32(0) && codehash != 0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470
avoid the hardcoded value check. This will cause problems in in future.
Be cautious with loops and array processing. If the number of interchainCalls
in the sendProposals
function is significant, it could cause the transaction to exceed the block gas limit, leading to a revert. Ensure that the gas limit is well within the block limit.
InterchainProposalSender.sol
contract doesn't have any nonce handling mechanism to prevent replay attacks. Consider adding a nonce or some other form of unique identifier to each proposal to prevent replay attacks
The contract inherits from AxelarExecutable, which likely implements a fallback function. Make sure to review the fallback function's implementation in the AxelarExecutable contract to ensure it behaves as expected
The contract ConstAddressDeployer.sol
uses create2 to deploy contracts. While this is generally safe and useful for deterministic contract deployments, be aware that contract deployment might consume more gas compared to standard create calls. Additionally, the size of the deployment bytecode should be kept in check to avoid potential out-of-gas issues
The contract uses assembly blocks to read and write from/to storage slots. While this can be an efficient way to handle storage operations, it increases the complexity of the code. Consider carefully evaluating the need for assembly and use it only when necessary to optimize gas costs
The _popExpressReceiveToken
and _popExpressReceiveTokenWithData
functions return the address expressCaller, but it is not used within the functions. If the return value is not needed, you can remove the assignment and just return early when the express caller is not found
When accessing storage slots with sload, be cautious about reading from slots that have not been written before. For example, you can initialize storage slots to a default value (e.g., 0) before using them
The _getExpressReceiveTokenSlot and _getExpressReceiveTokenWithDataSlot functions use keccak256 to calculate slots based on various parameters. Ensure that the combination of parameters used to calculate slots results in unique slots for each express call to avoid potential storage slot collisions
The contract inherits from Upgradable, which might imply that this contract is designed to be upgradable. Upgradable contracts can be complex and require careful consideration to maintain security and compatibility across versions
15 Hours
14 hours
#0 - c4-judge
2023-09-04T10:55:12Z
berndartmueller marked the issue as grade-b
#1 - sathishpic22
2023-09-08T19:12:46Z
Hi @berndartmueller
I received a lower grade than I expected, and I'm not sure what went wrong in my reports.
I wrote more detailed reports and followed the C4 format as instructed.
My report contains
I reviewed all the Grade A reports, and I believe my report is superior https://github.com/code-423n4/2023-07-axelar-findings/issues/439 , https://github.com/code-423n4/2023-07-axelar-findings/issues/376 , https://github.com/code-423n4/2023-07-axelar-findings/issues/206 to some of them.
Could you kindly reevaluate my report? If there are any areas that require improvement, I would greatly appreciate your feedback to enhance my analysis reports for future submissions.
Thank you
#2 - berndartmueller
2023-09-09T19:51:48Z
Hey @sathishpic22!
I'd like to point out that your submitted analysis report is an "advanced" report, and you compared it to "basic" analysis reports:
I reviewed all the Grade A reports, and I believe my report is superior https://github.com/code-423n4/2023-07-axelar-findings/issues/439 , https://github.com/code-423n4/2023-07-axelar-findings/issues/376 , https://github.com/code-423n4/2023-07-axelar-findings/issues/206 to some of them.
Nevertheless, after reviewing your submission again, I've decided to change the grade to "A". Please note that I see your report somewhere between grade "B" and "A". Mainly because your report seems more like a QA and Gas report smashed together. For an advanced analysis report to be graded A (or even selected as the primary report), it's important to analyze the codebase from a more general and high-level view and point out issues that are more high-level (see https://docs.code4rena.com/awarding/judging-criteria#analysis)
#3 - c4-judge
2023-09-09T19:52:03Z
berndartmueller marked the issue as grade-a