Cudos contest - GimelSec's results

Lines of code

https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L595-L609

Vulnerability details

Impact

Some ERC tokens have a fee on each transfer. The protocol doesn’t handle the fee when transferring this kind of ERC20 tokens, leading to the inconsistent amount of token actually received in the contract. Validators on the Cudos will mint more tokens to the destination address than it received in sendToCosmos().

Proof of Concept

In sendToCosmos, _amount is used in SendToCosmosEvent. However, if _tokenContract collects a fee on every transfer. The actual amount of token received is not equal to _amount

https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L595-L609

	function sendToCosmos(
		address _tokenContract,
		bytes32 _destination,
		uint256 _amount
	) public nonReentrant  {
		IERC20(_tokenContract).safeTransferFrom(msg.sender, address(this), _amount);
		state_lastEventNonce = state_lastEventNonce.add(1);
		emit SendToCosmosEvent(
			_tokenContract,
			msg.sender,
			_destination,
			_amount,
			state_lastEventNonce
		);
	}

Tools Used

vim

Recommended Mitigation Steps

Use balanceAfter - balanceBefore rather than using amount directly:

function retrieveTokens(address sender, uint256 amount) public {
  uint256 balanceBefore = deflationaryToken.balanceOf(address(this));
  deflationaryToken.transferFrom(sender, address(this), amount);
  uint256 balanceAfter = deflationaryToken.balanceOf(address(this));
  amount = balanceAfter.sub(balanceBefore);
  totalTokenTransferred += amount;
}

Token Transfer Calculation Accuracy in this article: https://blog.chain.link/defi-security-best-practices/

Lines of code

https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L337 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L406 https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L510

Vulnerability details

Impact

updateValset operation doesn't set a timeout, leading to the signed newCheckpoint (new Valset) being used indefinitely.

Proof of Concept

It doesn't check the operation timeout in the updateValset function, so the new Valset can be delayed and change the current Valset abruptly at any time.

Additionally, submitBatch and submitLogicCall function check the checkpoint:

require(
	makeCheckpoint(_currentValset, state_gravityId) == state_lastValsetCheckpoint,
	"Supplied current validators and powers do not match checkpoint."
);

An attacker can hold the newCheckpoint and call updateValset abruptly to change validators list to deny all valid operations, which has been signed by previous _currentValset.validators.

Tools Used

vim

Recommended Mitigation Steps

Use timeout in updateValset:

	function updateValset(
		// The new version of the validator set
		ValsetArgs memory _newValset,
		// The current validators that approve the change
		ValsetArgs memory _currentValset,
		// These are arrays of the parts of the current validator's signatures
		uint8[] memory _v,
		bytes32[] memory _r,
		bytes32[] memory _s,
	) public nonReentrant {
		// CHECKS

+		require(block.number < _newValset.updateTimeout, "Timed out");

		...

Save gas in for loops by unchecked arithmetic

The for loop has no overflow risk of i. Use an unchecked block to save gas.

Proof of Concept

Gravity.sol
128:             for (uint256 i = 0; i < _users.length; i++) {
233:            for (uint256 i = 0; i < _currentValidators.length; i++) {
263:            for (uint256 i = 0; i < _newValset.validators.length; i++) {
453:                            for (uint256 i = 0; i < _amounts.length; i++) {
568:            for (uint256 i = 0; i < _args.transferAmounts.length; i++) {
579:            for (uint256 i = 0; i < _args.feeAmounts.length; i++) {
660:            for (uint256 i = 0; i < _powers.length; i++) {

Recommendation

Use unchecked blocks to avoid overflow checks, or use ++i rather than i++ if you don't use unchecked blocks.

for (uint256 i = 0; i < length; ) {
    ...
    unchecked {
        ++i;
    }
}

Use immutable to save gas

Proof of Concept

https://github.com/code-423n4/2022-05-cudos/blob/main/solidity/contracts/Gravity.sol#L59-L61

Recommendation

	// These are set once at initialization
	bytes32 public immutable state_gravityId;
	uint256 public immutable state_powerThreshold;