Cudos contest - p_crypt0's results

Lines of code

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

Vulnerability details

Impact

Ability for admin to drain all ERC20 funds stored in contract at will, meaning all ERC20 based Cudos tokens (and any other ERC20 tokens stored in the contract) could be extracted by anyone with admin role and later sold, leaving users funds bridged on Cudos Cosmos chain with no ERC20 representation stored across the bridge - similar in impact as the wormhole hack.

This issue ought to fall within the limits the team allocated on assessing the governance role setups, since it describes a full-fledged security risk regarding users' funds. Crucially, this function is not in the original Gravity Bridge contract for Gravity.sol: https://github.com/Gravity-Bridge/Gravity-Bridge/blob/f65d9da692c1af76f8188bd17b55dea58c1d8723/solidity/contracts/Gravity.sol

Furthermore, the function has not been commented and does not appear in the documentation, suggesting that it has perhaps not yet been reasoned through by the development team and it's critical this is flagged in the security audit.

Proof of Concept

Firstly, User with admin role granted waits until CUDOS bridge has decent TVL from users bridging their CUDOS tokens from Ethereum to the CUDOS Cosmos chain,

Secondly, User manually calls withdrawERC20(address _tokenAddress) with the ERC token address of the CUDOS token

 function withdrawERC20(
			address _tokenAddress) 
			external {
			require(cudosAccessControls.hasAdminRole(msg.sender), "Recipient is not an admin");
			uint256 totalBalance = IERC20(_tokenAddress).balanceOf(address(this));
			IERC20(_tokenAddress).safeTransfer(msg.sender , totalBalance);
} 

Thirdly, withdrawERC20() function checks if user has admin role and if so withdraws all the tokens of a given token address straight to the admin's personal wallet

               require(cudosAccessControls.hasAdminRole(msg.sender), "Recipient is not an admin");
		uint256 totalBalance = IERC20(_tokenAddress).balanceOf(address(this));
		IERC20(_tokenAddress).safeTransfer(msg.sender , totalBalance);
		

Fourth, user exchanges CUDOS on DEX and then sends funds to tornado cash, leaving all user funds at risk.

Tools Used

My own logical reasoning and discussion with team on discord for confirmation of admin roles and function's logic.

Recommended Mitigation Steps

Delete the function or alternatively, send all funds to the '0' address to burn rather than give them to the admin.

Change withdrawERC20 to:

function burnERC20(
	address _tokenAddress) 
	external {
	require(cudosAccessControls.hasAdminRole(msg.sender), "Recipient is not an admin");
	uint256 totalBalance = IERC20(_tokenAddress).balanceOf(address(0));
	- IERC20(_tokenAddress).safeTransfer(msg.sender , totalBalance);
     +   IERC20(_tokenAddress).safeTransfer(address(0) , totalBalance);
}

Lines of code

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

Vulnerability details

Impact

No checks for non-Cudos tokens mean that non-Cudos ERC20 tokens will be lost to the contract, with the user not having any chance of retrieving them.

However, the admin can retrieve them through withdrawERC20.

Impact is that users lose their funds, but admins gain them.

The mistakes could be mitigated on the contract, by checking against a list of supported tokens, so that users don't get the bad experience of losing funds and CUDOS doesn't have to manually refund users

Proof of Concept

User sends 100 ETH through sendToCosmos, hoping to retrieve 100 synthetic ETH on Cudos chain but finds that funds never appear.

	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
		);
	}

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

Admin can retrieve these funds should they wish, but user never gets them back because the contract does not check whether the token is supported.

	function withdrawERC20(
		address _tokenAddress) 
		external {
		require(cudosAccessControls.hasAdminRole(msg.sender), "Recipient is not an admin");
		uint256 totalBalance = IERC20(_tokenAddress).balanceOf(address(this));
		IERC20(_tokenAddress).safeTransfer(msg.sender , totalBalance);
	}

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

Tools Used

Logic and discussion with @germanimp

Recommended Mitigation Steps

Add checks in sendToCosmos to check the incoming tokenAddress against a supported token list, so that user funds don't get lost and admin don't need to bother refunding.