code-423n4 / 2022-05-cudos-findings

1 stars 0 forks source link

The validators can rug users by transferring funds from users' wallets #100

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

https://github.com/code-423n4/2022-05-cudos/blob/de39cf3cd1f1e1cf211819b06d4acf6a043acda0/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
    );
}

In sendToCosmos(), funds are pulled from user's wallet to the contract using safeTransferFrom(), which means the contract will hold user's allowance.

In the meantime, the validators can collectively execute arbitrary code with submitLogicCall().

Combine the above two, the validators will be able to pull funds from any user's wallet as long as the user has given approval to the contract.

If the validators got enough profit to cover the potential slashing of their staked tokens, it will be in their best interest to initiated such an attack.

Thus, we should not make it possible for the validators to do so.

Recommendation

Consider create a separate contract to hold the funds/allowances from the users.

V-Staykov commented 2 years ago

This is indeed an issue, but we have removed usage of submitLogicCall, so it is redundant. At most we would probably remove it as a whole.

mlukanova commented 2 years ago

PR: https://github.com/CudoVentures/cosmos-gravity-bridge/pull/16

CloudEllie commented 2 years ago

Duplicate of #72 (per judge)

Pedroais commented 2 years ago

This requires a majority vote of the validators to go be exploited. If a majority of the validators were malicious then, of course, the whole bridge could always be exploited.

This shouldn't be high severity since there are external requirements (malicious validators).

I think it should be invalid and not even medium severity since the idea of the bridge is that there are many validators and you need a majority to steal the funds. This is the bridge working as intended.