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: 4/13
Findings: 3
Award: $6,956.00
🌟 Selected for report: 3
🚀 Solo Findings: 1
1268.3578 USDC - $1,268.36
shw
The updateValset function does not check whether the new valset has sufficient power to pass a vote (see the constructor for more details). If the new valset does not, any function calling checkValidatorSignatures will be disabled (since the transaction reverts).
Referenced code: Gravity.sol#L224 Gravity.sol#L584-L590
Add a check to ensure that the total power of the new valset is at least the power threshold.
#0 - jkilpatr
2021-09-09T11:28:40Z
This is a good bug report highlighting a real oversight. We do check that all validator powers add up to the expected amount on the Gravity module side but there's no reason not to perform that same check on this side.
I would describe this bug as high risk but low probability since it would require this normalization code to fail as well.
Semi duplicate in #51 which also describes this issue. Duplicate of #37
🌟 Selected for report: shw
4697.6215 USDC - $4,697.62
shw
The sendToCosmos function of Gravity transfers _amount of _tokenContract from the sender using the function transferFrom. If the transferred token is a transfer-on-fee/deflationary token, the actually received amount could be less than _amount. However, since _amount is passed as a parameter of the SendToCosmosEvent event, the Cosmos side will think more tokens are locked on the Ethereum side.
Referenced code: Gravity.sol#L535 Gravity.sol#L541
Consider getting the received amount by calculating the difference of token balance (using balanceOf) before and after the transferFrom.
#0 - jkilpatr
2021-09-09T15:18:19Z
This is a valid issue, it does present the ability to 'steal' tokens from the bridge, so I think that justifies the severity.
If user (A) deposits a deflationary token and gets slightly more vouchers than where actually deposited into the bridge upon withdraw they could steal tokens from user (B) who had also deposited.
#1 - loudoguno
2021-10-01T03:47:02Z
reopening as per judges assessment as "primary issue" on findings sheet
285.3805 USDC - $285.38
shw
The verifySig function of Gravity calls the Solidity ecrecover function directly to verify the given signatures. However, the ecrecover EVM opcode allows malleable (non-unique) signatures and thus is susceptible to replay attacks.
Although a replay attack seems not possible here since the nonce is increased each time, ensuring the signatures are not malleable is considered a best practice (and so is checking _signer != address(0), where address(0) means an invalid signature).
Referenced code: Gravity.sol#L153
SWC-117: Signature Malleability SWC-121: Missing Protection against Signature Replay Attacks
Use the recover function from OpenZeppelin's ECDSA library for signature verification.
#0 - jkilpatr
2021-09-09T15:25:41Z
Best practicies advice may belong in category zero. But in general I agree with the advice here and that this is valid feedback despite lacking a specific attack vector.
semi-duplicate of #43, #28 which mention the validation issue. #22 also mentions malleability.
#1 - albertchon
2021-09-23T13:27:02Z
Marking https://github.com/code-423n4/2021-08-gravitybridge-findings/issues/61 as primary for the signature malleability issue.
Duplicates:
#2 - loudoguno
2021-10-01T03:47:18Z
reopening as per judges assessment as "primary issue" on findings sheet
704.6432 USDC - $704.64
shw
SafeMath library functions are not always used in the Gravity contract's arithmetic operations, which could cause integer underflow/overflows. Using SafeMath is considered a best practice that could completely prevent underflow/overflows and increase code consistency.
Referenced code: Gravity.sol#L202 Gravity.sol#L586
Consider using the SafeMath library functions in the referenced lines of code.
#0 - jkilpatr
2021-09-09T15:28:18Z
An overflow in the powers would be a significant bug, while it would require some pretty dramatic issues no the go module side there is value in checking in. I agree with the severity
duplicate of #38
#1 - loudoguno
2021-10-01T03:48:53Z
reopening as per judges assessment as "primary issue" on findings sheet