Canto contest - ronnyx2017's results

Lines of code

https://github.com/code-423n4/2022-11-canto/blob/main/Canto/x/csr/keeper/evm_hooks.go#L51-L61

Vulnerability details

Impact

The target contract of the fee distribution is got by contract := msg.To() in the evm_hooks.go . So the fee distribution is only for the msg.to contract, instead of the to address of the call traces. It means that any one use a contract wallet or setup a proxy for the specified contracts will save lots of gas and can not share the recycled gas with the contract developers, which is not reasonable, even harmful for the gas economic system.

Proof of Concept

I write a demo with foundry. Because foundry cant sync blocks during broadcast, the following tests are not very concise.

First prepare the genesis.json with enabled csr module. Modify the init_testnet.sh, add this line after cantod init:

cat $HOME/.cantod/config/genesis.json | jq '.app_state["csr"]["params"]["enable_csr"]=true' > $HOME/.cantod/config/tmp_genesis.json && mv $HOME/.cantod/config/tmp_genesis.json $HOME/.cantod/config/genesis.json

start test net,run ./init_testnet.sh in the Canto dir.

Test: CIP-001/script/MyTest.3.s.sol:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.17;

import "forge-std/Script.sol";
import "../src/Turnstile.sol";

contract ReceiveSmartContract {
    bytes32 hh;
    function register(address _r) external returns (uint256 tokenId){
        Turnstile turnstile = Turnstile(0xEcf044C5B4b867CFda001101c617eCd347095B44);
        return turnstile.register(_r);
    }
    function burner() external returns (bytes32){
        bytes32 h;
        for (uint i = 0; i < 10; i++) {
            h = keccak256(abi.encode(0xffffffffffffffffffffffffffffffff));
        }
        hh = h;
        return h;
    }
    receive() external payable {}
}

contract Enter{
    function register(address _r) external returns (uint256 tokenId){
        Turnstile turnstile = Turnstile(0xEcf044C5B4b867CFda001101c617eCd347095B44);
        return turnstile.register(_r);
    }
    function burner(ReceiveSmartContract _e) external returns (bytes32){
        return _e.burner();
    }
    receive() external payable {}
}

contract Viewer{
    event balance(uint indexed tokenId, uint indexed balance);
    Turnstile turnstile = Turnstile(0xEcf044C5B4b867CFda001101c617eCd347095B44);
    function log(uint tokenId) external{
        emit balance(tokenId, turnstile.balances(tokenId));
    }
}

contract PwnTest is Script {
    function setUp() public {}

    function run() external {
        uint256 mykey = 0x32b; // here should be `cantod keys export mykey --unsafe  --unarmored-hex`;
        address myaddr = vm.addr(mykey);
        vm.startBroadcast(mykey);
        Viewer v = new Viewer();
        ReceiveSmartContract rsc = new ReceiveSmartContract();
        Enter enter = new Enter();
        
        uint256 tokenId1 = rsc.register(myaddr);
        uint256 tokenId2 = enter.register(myaddr);
        console.log(tokenId1, tokenId2);
        v.log(tokenId1);
        v.log(tokenId2);
        enter.burner(rsc);
        v.log(tokenId1);
        v.log(tokenId2);
        vm.stopBroadcast();
    }
}

run script:

forge script script/MyTest.3.s.sol:PwnTest --rpc-url http://127.0.0.1:8545 --broadcast --slow --skip-simulation

The transactions will be saved to run-latest.json, and you can find the balance events which show that the balance of the token 0(ReceiveSmartContract) is not changed after calling enter.burner(rsc);, but only the balance of the token 1 (Enter) will increase after the fee distribution.

Tools Used

foundry

Recommended Mitigation Steps

follow the call traces

Lines of code

https://github.com/code-423n4/2022-11-canto/blob/main/Canto/x/csr/keeper/event_handler.go#L30-L33

Vulnerability details

Impact

The check about if the receiver account exists in the evm store doesn't make sense and will cause users to encounter a confusing exception. And the RegisterEvent function will not throw an exception to revert the tx, the source contract will be wrote in the feeRecipient mapping of the Turnstile contract successfully, but cant be write to the store of the Keeper. So the users contracts will be blocked forever by the onlyUnregistered in the Turnstile contract.

Proof of Concept

The check in the event_handler.go try to find out if the recipient address is in the evm store db.

    if acct := k.evmKeeper.GetAccount(ctx, event.Recipient); acct == nil {
		return sdkerrors.Wrapf(ErrNonexistentAcct, "EventHandler::RegisterEvent account does not exist: %s", event.Recipient)
	}

But the address is saved to the db only when it has sent/received a tx or it's a valid contract. If users set a new EOA address as the receiver, I think it's common and makes sense, the register will failed without prompt and the source contract cant be registered forever. And, only for the implementation logic of this check, the address state is not immutable. The contract address can be deleted from state db when it's suicided.

Tools Used

Recommended Mitigation Steps

Remove this check