Platform: Code4rena
Start Date: 29/07/2022
Pot Size: $50,000 USDC
Total HM: 6
Participants: 75
Period: 5 days
Judge: GalloDaSballo
Total Solo HM: 3
Id: 149
League: ETH
Rank: 21/75
Findings: 2
Award: $101.29
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: oyc_109
Also found by: 0x1f8b, 0x52, 0xNazgul, 0xSmartContract, 0xf15ers, 8olidity, Aymen0909, Bnke0x0, CertoraInc, Chom, CodingNameKiki, Deivitto, Dravee, ElKu, IllIllI, JC, Lambda, Noah3o6, NoamYakov, RedOneN, Respx, ReyAdmirado, Rohan16, Rolezn, Ruhum, Sm4rty, TomJ, Twpony, Waze, Yiko, __141345__, ajtra, apostle0x01, ashiq0x01, asutorufos, bardamu, benbaessler, berndartmueller, bharg4v, bulej93, c3phas, cccz, ch13fd357r0y3r, codexploder, cryptonue, cryptphi, defsec, djxploit, durianSausage, fatherOfBlocks, gogo, hansfriese, horsefacts, ignacio, kyteg, lucacez, mics, rbserver, robee, sashik_eth, simon135, sseefried, tofunmi, xiaoming90
70.0708 USDC - $70.07
Some tokens (like USDT L199) do not work when changing the allowance from an existing non-zero allowance value. They must first be approved by zero and then the actual allowance must be approved.
A good practice is to use constant variables instead of hardcoded strings in the code.
For instance, [DepositBase.sol#L31](https://github.com/code-423n4/2022-07-axelar/tree/main/C:\Users\eldad\Desktop\Code Arena\2022-07-axelar\contracts/deposit-service/DepositBase.sol#L31)
According to the ERC20 specs, the approve function is allowed to return a success value, that may be negative. Same as transfer return value, approve return value should be handled.
Some contracts does not support 0 transfer, then the transaction will revert with no explanation. We recommend to add a require statement that the amount is not 0.
Use openzeppelin safeApprove() method instead of approve() in the following locations.
Use openzeppelin safeTransfer() method instead of transfer() in the following locations.
The following calls ignores the return value of the called function that might indicate the the call failed.
The project is compiled with different versions of solidity, which is not recommended because it can lead to undefined behaviors.
In case a hack is occuring or an exploit is discovered, the team (or validators in this case) should be able to pause functionality until the necessary changes are made to the system. Additionally, the gravity.sol contract should be manged by proxy so that upgrades can be made by the validators.
Because an attack would probably span a number of blocks, a method for pausing the contract would be able to interrupt any such attack if discovered.)
The process of transferring ownership is dangerous since typing the wrong address can lead to severe implications. It is better to have to steps verification process with set and claim functions to decrease the chances of human error. Consider changing to two steps verification process of transferring privileges. Human mistakes can happen.
There is no check for the access to be in the array bounds.
For instance, AxelarAuthWeighted.sol
The following functions allows attackers to try reentrancy since they are calling to external contracts / transferring eth. Consider adding a nonReentrancy modifier.
It is good to have a timelock for functions that set key/critical variables.
A state variable of type 'address' is set without a non-zero verification. This can lead to undesired behavior.
When changing state variables events are not emitted. Emitting events allows monitoring activities with off-chain monitoring tools.
The following functions are payable but doesn't record the sender transaction. Consider making them not payable instead.
In functions that update/assigns state variables it is a good practice to emit event.
If for any reason the following unused parameters are necessary then remove their naming (since only the type matters for function signature)
The fee parameter should be validated to me strictly greater than 0 and less than 100%. In the following lines at least one of those checks is missing.
#0 - re1ro
2022-08-05T01:38:04Z
Dup #3
Ack. We might keep it this way because those are error messages.
Good spot
Yup. Dup #16
Yup. Good spot
Yup. Good spot.
Not applicable.
Not applicable.
Dup #8
No applicable by design
Dup #16
Not applicable. It's checked in _transferOperatorship
Good spot.
Ack
There is not reentrancy threat there
Ack
Good spot
Not applicable. Those are example/unit test contracts
Not applicable. Those are example/unit test contracts. Payable functions use less gas and actually needed in those places.
Not applicable
Ack
Ack
#1 - GalloDaSballo
2022-09-05T14:46:23Z
Disputed as it's used properly in this codebase
R, this is another issue of using strings instead of custom errors
R
L
NC
Disagree with blank-all statement
NC
R as the statement is closer to "fail early"
L
NC
You can't make a statement about security and then link to the entire file, disputed in lack of a clear risk
Disputed per the rulebook, you cannot verify a timelock is or is not being used
NC
Every other finding doesn't link to the specific line nor mentions it, for that reason I'm ignoring them.
A report like this feels like hearing Rajeev from Secureum mixed in with other old reports, I don't think it offers anything unique to the sponsor
The lack of polish (links without reference, dump of links) further diminishes the perceived value from this report, which does have some valid findings.
I'm not impressed
2L 3R 4NC
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0xNazgul, 0xsam, 8olidity, Aymen0909, Bnke0x0, Chom, CodingNameKiki, Deivitto, Dravee, ElKu, Fitraldys, JC, Lambda, MiloTruck, Noah3o6, NoamYakov, RedOneN, Respx, ReyAdmirado, Rohan16, Rolezn, Ruhum, Sm4rty, TomJ, Tomio, Waze, __141345__, a12jmx, ajtra, ak1, apostle0x01, asutorufos, benbaessler, bharg4v, bulej93, c3phas, defsec, djxploit, durianSausage, erictee, fatherOfBlocks, gerdusx, gogo, kyteg, lucacez, medikko, mics, owenthurm, oyc_109, rbserver, robee, sashik_eth, simon135, tofunmi
31.222 USDC - $31.22
In the following for loops consider caching the array size instead of loading it every iteration.
You can replace a<=b with a-1<b which reduces the gas usage.
reading msg.sender is 2 gas units which is less than a read of a local var + the unnecessary store operation.
In order to save gas you can put a payable modifier for functions that are called by protocol owners.
#0 - re1ro
2022-08-05T01:45:35Z
abi.encodePacked is not secure. Dup #2
Yup. Dup #2
replace <= and >=
Good spot.
Not applicable. Broken line numbers...
Ack. We might prefer readability over this optimisation.
Dup #7 [G-05]
#1 - GalloDaSballo
2022-08-23T00:55:17Z
Less than 100 gas saved
iszero doesn't always save gas when compared to unchecked
unchecked almost always is equivalent, true benchmark should be offered for this type of advice as the blanket statement is a 50/50