L-01 : Direct usage of ecrecover allows signature malleability
L-02 : SafeMath library is not always used in Gravity.sol
L-03 : Input validation of constructor parameters in Gravity.sol
L-04 : Use structure for signature
L-05 : Use constant for MAX_UINT
Details L-01
Title : Direct usage of ecrecover allows signature malleability
Impact
The verifySig function of Gravity.sol 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).
Proof of Concept
Contract : Gravity.sol
Function : verifySig
Recommended Mitigation Steps
Use the recover function from OpenZeppelin's ECDSA library for signature verification.
Details L-02
Title : SafeMath library is not always used in Gravity.sol
Impact
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.
Proof of Concept
Contract : Gravity.sol
Function : checkValidatorSignatures(...)
line 244 : cumulativePower = cumulativePower + _currentPowers[i];
Function : constructor(...)
line 661 : cumulativePower = cumulativePower + _powers[i];
Recommended Mitigation Steps
Consider using the SafeMath library functions in the above mentioned lines of code.
Details L-03
Title : Input validation of constructor parameters in Gravity.sol
In the constructor additional check for _gravityId not to be null, and _powerThreshold to be greater than 0, can be added.
Proof of Concept
Contract : Gravity.sol
Function : constructor (...)
Recommended Mitigation Steps
Add appropriate require statement checking the input parameters mentioned above for the constructor
Details L-04
Title : Use structure for signature
Add a struct "Signature" which contains the v, r, and s components of a validator signature, This is a more compact encoding and
removes the possibility of mismatched signature array lenghts.
Furthermore this reduces our total stack usage and allows us to declare validator sets and signatures to be of type 'calldata' universally.
// This represents a validator signature
struct Signature {
uint8 v;
bytes32 r;
bytes32 s;
}
Summary of Findings for Low / Non-Critical issues
Details L-01
Title : Direct usage of ecrecover allows signature malleability
Impact
The verifySig function of Gravity.sol 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).
Proof of Concept
Contract : Gravity.sol Function : verifySig
Recommended Mitigation Steps
Use the recover function from OpenZeppelin's ECDSA library for signature verification.
Details L-02
Title : SafeMath library is not always used in Gravity.sol
Impact
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.
Proof of Concept
Contract : Gravity.sol Function : checkValidatorSignatures(...) line 244 : cumulativePower = cumulativePower + _currentPowers[i];
Function : constructor(...) line 661 : cumulativePower = cumulativePower + _powers[i];
Recommended Mitigation Steps
Consider using the SafeMath library functions in the above mentioned lines of code.
Details L-03
Title : Input validation of constructor parameters in Gravity.sol
In the constructor additional check for _gravityId not to be null, and _powerThreshold to be greater than 0, can be added.
Proof of Concept
Contract : Gravity.sol Function : constructor (...)
Recommended Mitigation Steps
Add appropriate require statement checking the input parameters mentioned above for the constructor
Details L-04
Title : Use structure for signature
Add a struct "Signature" which contains the v, r, and s components of a validator signature, This is a more compact encoding and removes the possibility of mismatched signature array lenghts.
Furthermore this reduces our total stack usage and allows us to declare validator sets and signatures to be of type 'calldata' universally.
// This represents a validator signature struct Signature { uint8 v; bytes32 r; bytes32 s; }
Proof of Concept
Contract : Gravity.sol Functions : testCheckValidatorSignatures, verifySig, checkValidatorSignatures, updateValset, submitBatch, submitLogicCall
Recommended Mitigation Steps
Refer to the code fix in https://github.com/Gravity-Bridge/Gravity-Bridge/commit/2bd0837b3ef52fe2f2abc705fc72f371e047c857
Details L-05
Title : Use constant for MAX_UINT
Use constants to improve security by preventing assignments and increase performance by explicitly flagging this constant.
Proof of Concept
Contract : CosmosERC20 in CosmosToken.sol Line 5 : uint256 MAX_UINT = 2**256 - 1;
Recommended Mitigation Steps
Change the above line to
uint256 constant MAX_UINT = 2**256 - 1;