Platform: Code4rena
Start Date: 26/08/2021
Pot Size: $100,000 USDC
Total HM: 8
Participants: 13
Period: 14 days
Judge: Albert Chon
Total Solo HM: 7
Id: 27
League: COSMOS
Rank: 3/13
Findings: 3
Award: $8,712.02
🌟 Selected for report: 7
🚀 Solo Findings: 0
1268.3578 USDC - $1,268.36
pauliax
The code does not enforce that the sum of validator powers is no less than the threshold. It is possible that even when all validators sign the message their total power is not enough to confirm it.
While this may increase the gas usage I advise you to sum the total power and check that it can reach the threshold when setting or updating validators and powers.
#0 - jkilpatr
2021-09-10T14:45:31Z
duplicate of #63 and semi duplicate of #51
422.7859 USDC - $422.79
pauliax
Consider introducing upper and lower limits for the size of the validators array. Currently, there are no restrictions on validators array. In theory, a scenario exists where there are too many validators in the array with low individual powers and a high threshold so the loop that iterates over them will never finish (exceeds block gas limit). I advise setting boundaries for the size of validators (e.g. MIN_VALIDATORS and MAX_VALIDATORS). Every chain has its gas limit so you can individually test the capacity and then set it via the constructor. The lower limit could be set to 1 so that it wouldn't be possible to have 0 validators. It could also check that addresses are unique, no the same validator is present twice.
Introduce validations for the array of validators.
#0 - jkilpatr
2021-09-10T15:13:44Z
I had not considered this issue. From some rough napkin math a validator set update with 125 validators costs 706,459 gas, assuming linear scaling and given a block gas limit of 30 million that's ~6000 validators. Even if they cut the block gas limit by ~80% we're still looking at an enormous gas margin versus normal cosmos validator set sizes.
Good report, but I believe this a wontfix
#1 - albertchon
2021-09-23T14:04:21Z
Cosmos-SDK based chains typically have < 200 validators so not a huge issue
704.6432 USDC - $704.64
pauliax
I am concerned why invalidationId, invalidationNonce or valsetNonce are only required to be greater than the previous value. Why did you choose this approach instead of just simply asking for an incremented value? While this may not be a problem if the validators are honest, but otherwise, they may submit a nonce of MAX UINT and thus block the whole system as it would be no longer possible to submit a greater value. Again, just wanted you to be aware of this issue, not sure how likely this to happen is in practice, it depends on the honesty of validators so you better know.
I didn't receive an answer on Discord so decided to submit this FYI to decide if that's a hazard or no.
#0 - jkilpatr
2021-09-10T14:52:41Z
This is frankly a good point. We need to be able to skip nonces because we may sometimes create batches that are not profitable or validator sets that we don't want to bother submitting.
A reasonable mitigation for this issue would be to limit how far ahead we can skip at any one time. Preventing error or hostile action from locking funds in the bridge forever by setting the maximum nonce.
#1 - albertchon
2021-09-23T13:36:34Z
Well this actually isn't an accurate attack since the nonce representation on the module side is uint64 whereas it's a uint256 on the cosmos side.
Still, I think this attack is quite hard to trigger naturally since the nonce is incremented by 1 on the module side and as sending 2^64 -1 transactions on a Cosmos chain (to trigger this overflow) would be cost prohibitive/
And I think attacks that assume validators collude aren't really attacks since that's the natural trust assumption already.
Marking this one as the primary.
#2 - loudoguno
2021-10-01T03:47:29Z
reopening as per judges assessment as "primary issue" on findings sheet
🌟 Selected for report: pauliax
1565.8738 USDC - $1,565.87
pauliax
There are a few validations that could be added to the system: the constructor could check that _gravityId is not empty. state_powerThreshold should always be greater than 0, otherwise, anyone will be available to execute actions.
Consider implementing suggested validations.
#0 - jkilpatr
2021-09-10T14:53:44Z
These are good suggestions. In my opinion powerThreshold should probably just be hard coded at this point. GravityID being empty is not a vulnerability I had considered before.
#1 - loudoguno
2021-10-01T03:48:40Z
reopening as per judges assessment as "primary issue" on findings sheet
704.6432 USDC - $704.64
pauliax
When calculating cumulativePower regular arithmetic operations (not SafeMath) are used. In theory, this can result in value overflows, however, in practice, this depends on the honesty of those that can assign powers (e.g. when deploying or updating valset).
Make sure that you understand this risk and consider using SafeMath operations there.
#0 - jkilpatr
2021-09-10T14:39:13Z
duplicate of #60
🌟 Selected for report: pauliax
1565.8738 USDC - $1,565.87
pauliax
Based on my understanding cumulativePower checks should be inclusive to indicate when the threshold is met. Otherwise, there might be impossible to reach it in certain cases (e.g. when 100% power is required). Replace '>' with '>=' in constructor and function checkValidatorSignatures: if (cumulativePower > _powerThreshold) { break; } require( cumulativePower > _powerThreshold, "Submitted validator set signatures do not have enough power." );
cumulativePower >= _powerThreshold
#0 - jkilpatr
2021-09-10T15:19:00Z
I would classify this as low risk since the bridge would never in any sane situation be configured to require 100% of the power. It's a valid report in the context that a slightly more permissive check could save the day in very specific situations.
#1 - loudoguno
2021-10-01T03:49:08Z
reopening as per judges assessment as "primary issue" on findings sheet
🌟 Selected for report: pauliax
524.714 USDC - $524.71
pauliax
Function makeCheckpoint expects that powers in ValsetArgs are in decreasing order: However, it seems that this is not enforced anywhere in the code and is left for the caller's responsibility. // The validator powers must be decreasing or equal. This is important for checking the signatures on the // next valset, since it allows the caller to stop verifying signatures once a quorum of signatures have been verified. function makeCheckpoint(
This can be enforced in the smart contract when setting or updating valset by checking that the current power is less or equal than the previous one.
#0 - jkilpatr
2021-09-10T15:17:21Z
I would classify this as an optimization. Decreasing order is a nice optimization but it's totally optional to actually use, enforcing it is another error case that's not really needed.
#1 - loudoguno
2021-10-01T03:54:39Z
reopening as per judges assessment as "primary issue" on findings sheet
285.3805 USDC - $285.38
pauliax
There is a common issue that ecrecover returns empty (0x0) address when the signature is invalid. While I didn't find any exact exploit path in your codebase, I still wanted to submit this as in a previous contest similar issue was assigned a high severity even no exact attack path and poc existed (see https://github.com/code-423n4/2021-04-meebits-findings/issues/4).
Just wanted you to be aware of this as you may decide to add a check against an empty address or the judge can mark this as invalid otherwise.
#0 - jkilpatr
2021-09-10T15:14:48Z
duplicate of #43, #61,#21
#1 - albertchon
2021-09-23T13:27:19Z
🌟 Selected for report: pauliax
524.714 USDC - $524.71
pauliax
You may want to consider calling logicContractAddress only when the payload is not empty: bytes memory returnData = Address.functionCall(_args.logicContractAddress, _args.payload);
You can do the external call only if _args.payload is not empty.
#0 - jkilpatr
2021-09-10T14:57:55Z
I would classify this as a gas optimization. Otherwise good.
#1 - loudoguno
2021-10-01T03:54:45Z
reopening as per judges assessment as "primary issue" on findings sheet
🌟 Selected for report: pauliax
524.714 USDC - $524.71
pauliax
_currentValset.validators.length in functions submitLogicCall, submitBatch and updateValset are accessed multiple (4) times. It would save some gas if you cache this value in a variable and re-use it where necessary.
#0 - jkilpatr
2021-09-10T14:48:21Z
Caching requires using a register, which we're running quite low on due to other variables and scoping. We'll see if I can make this work out.
#1 - loudoguno
2021-10-01T03:54:50Z
reopening as per judges assessment as "primary issue" on findings sheet
pauliax
Variables that do not change can be marked as immutable. This greatly reduces gas cots. Examples of such variables are: // These are set once at initialization bytes32 public state_gravityId; uint256 public state_powerThreshold;
#0 - jkilpatr
2021-09-10T14:49:16Z
duplicate of #48
🌟 Selected for report: pauliax
524.714 USDC - $524.71
pauliax
Structs LogicCallArgs and ValsetArgs can be optimized for storage efficiency. Struct variables are stored in 32 bytes each so you can group smaller types to occupy less storage. You can read more here: https://fravoll.github.io/solidity-patterns/tight_variable_packing.html or in the official documentation: https://docs.soliditylang.org/en/v0.4.21/miscellaneous.html For example, I think timeOut member does not need to be uint256 as block number is unlikely to reach this capacity anytime soon. Even amounts can be reduced to uint112 as that is what Uniswap and similar AMM uses and they work just fine. I can't tell the exact optimal order, you will need several iterations of changing and testing to find it (probably with the help of gasReporter) but I just wanted you to be aware of this possibility to reduce tx fees.
#0 - jkilpatr
2021-09-10T14:47:26Z
Appreciate the report here. Probably will not pursue compared to some of the other optimizations offered, but it's good to know it's possible.
Timeout being a u128 is probably viable, or even u64
#1 - loudoguno
2021-10-01T03:54:58Z
reopening as per judges assessment as "primary issue" on findings sheet