Cudos contest - 0x1337's results

Lines of code

https://github.com/code-423n4/2022-05-cudos/blob/de39cf3cd1f1e1cf211819b06d4acf6a043acda0/solidity/contracts/Gravity.sol#L660-L669 https://github.com/code-423n4/2022-05-cudos/blob/de39cf3cd1f1e1cf211819b06d4acf6a043acda0/solidity/contracts/Gravity.sol#L276-L358

Vulnerability details

Impact

The constructor of the Gravity contract checks that cumulativePower is greater than _powerThreshold. However, the updateValset() function contains no check that cumulativePower > state_powerThreshold for the new validator set. The missing check is dangerous because if the new validator set does not contain sufficient cumulative voting power, then all function calls that require the successful execution of checkValidatorSignatures() function would fail, rendering the gravity contract unusable.

Note that this problem cannot be fixed by the validators subsequently signing off on a new validator set that does satisfy the condition, because the updateValset() function also requires the successful execution of the checkValidatorSignatures() function.

Proof of Concept

In the constructor of the Gravity contract, the following lines of code check that cumulativePower is greater than _powerThreshold. The state_powerThreshold variable then records the _powerThreshold of the validator set.

https://github.com/code-423n4/2022-05-cudos/blob/de39cf3cd1f1e1cf211819b06d4acf6a043acda0/solidity/contracts/Gravity.sol#L660-L669

However, in the updateValset() function, there is no check that the cumulativePower of the _newValset is still greater than state_powerThreshold, which could potentially result in insufficient cumulativePower for the _newValset, in which case the submitBatch() and submitLogicCall() functions would always revert, because the checkValidatorSignatures() function would be guaranteed to fail.

Tools Used

Manual review

Recommended Mitigation Steps

Recommend adding a check that the cumulativePower of the _newValset is still greater than state_powerThreshold in the updateValset() function, similar to the check in the constructor.

Lines of code

https://github.com/code-423n4/2022-05-cudos/blob/de39cf3cd1f1e1cf211819b06d4acf6a043acda0/solidity/contracts/Gravity.sol#L632-L638

Vulnerability details

Impact

If one admin of the cudosAccessControls contract is hacked or compromised, the entire value locked and all user fund in the bridge could be lost on both the Ethereum side and the Cudos side.

Proof of Concept

The security design of the bridge is that a sufficient number of validators need to sign a transaction, in order for the gravity contract to execute a transaction via the submitBatch() or the submitLogicCall() function. These functions are used to transfers tokens from addresses on the Cudos chain to addresses on the Ethereum chain, or execute other logic on the Ethereum side.

However, the withdrawERC20() function completely bypasses this security design. Any address with the admin role in the cudosAccessControls contract can call withdrawERC20(), and drain any ERC20 token balance from the gravity contract. These tokens might either be users' deposit into the gravity contract (via the sendToCosmos() function), or synthetic tokens that represent tokens deposited and bridged on the Cudos chain. In the first scenario, users' fund on the Ethereum chain could be lost. In the second scenario, a compromised admin can bridge the synthetic tokens back to the Cudos chain, by calling sendToCosmos() and using the event emitted to withdraw from the Cudos chain, and users fund on the Cudos chain would be lost. As a result, the total value locked in the bridge on both chains could be lost if one admin is compromised or hacked.

https://github.com/code-423n4/2022-05-cudos/blob/de39cf3cd1f1e1cf211819b06d4acf6a043acda0/solidity/contracts/Gravity.sol#L632-L638

Tools Used

Manual Review

Recommended Mitigation Steps

The withdrawERC20() function should only be used to withdraw tokens sent to the contract by accident, and not users deposit or synthetic tokens that represent tokens deposited on the Cudos chain. Recommend adding a check in this function that the _tokenAddress cannot represent any token that uses deposit or tokens deposited on the Cudos chain.

Alternatively, the same security model should be applied to the withdrawERC20() function, which means that there needs to be validator signature verification in this function, just like in the submitBatch() and the submitLogicCall() functions.

The whitelisted variable, onlyWhitelisted modifier, and the manageWhitelist function are declared in the gravity contract, and yet do not play any role in the functioning of the bridge.

https://github.com/code-423n4/2022-05-cudos/blob/de39cf3cd1f1e1cf211819b06d4acf6a043acda0/solidity/contracts/Gravity.sol#L116-L136

Recommend removing unused variables, modifiers, and functions.