everx-labs / TVM-Solidity-Compiler

Solidity compiler for TVM
GNU General Public License v3.0
125 stars 72 forks source link

Proposal: force users to explicitly specify responsible answer flags. #87

Closed mnill closed 1 year ago

mnill commented 2 years ago

Hi there,

I will start with two problems with default responsible.

Problem #1, default responsible has flag: 0 and he send value depend on pragma msgValue? 7_500_000 or 9_500_000.

Simple contract:

contract SimpleContract {
    address static owner;

    constructor() public {
        tvm.accept();
    }

    function get_owner() external pure responsible returns (address) {
        return owner;
    }
}

Get owner will be compiled to

function get_owner() external pure responsible returns (address) {
    return {flag: 0, value: 7_500_00, bounce: true} owner;
 } 

Also it can be used to make money. If attacker find contract with such responsible he can pay for call responsible < 7_500_00 evers and get back 7_500_00 evers. So he can drain balance. This will getting worse if gas price will be lower in future.

Problem #2, default responsible has bounce: true, and it is really bad.

Example of vulnerable contract (pseudocode):

pragma ton-solidity >= 0.39.0;
pragma AbiHeader expire;
pragma AbiHeader pubkey;

contract VulnerableContract  {
    uint128 balance;

    function get_balance() override external view responsible returns (uint32) {
        return{flag: 64, value: 0} balance;
    }

    function transfer(uint128 _amount_tokens, uint256 _to_pubkey) external {
        require(tvm.pubkey() == msg.pubkey());
        require(balance > _amount_tokens);

        tvm.accept();
        address reciever = _getExpectedAddress(_to_pubkey); 
        balance -= _amount_tokens;
        VulnerableContract(reciever).recieve{value: 7_500_000, flag: 0, bounce: true}(_amount_tokens, tvm.pubkey());
    }

    function recieve(uint128 _amount_tokens, uint256 _from_pubkey) {
        address expected_sender = _getExpectedAddress(_from_pubkey)
        require(msg.sender == expected_sender);
        balance += _amount_tokens
    }

    onBounce(TvmSlice body) external {
        tvm.accept();
        uint32 functionId = body.decode(uint32);
        if (functionId == tvm.functionId(VulnerableContract.transfer)) {
            uint128 amount_tokens = body.decode(uint128);
            balance += amount_tokens;
        }
    }
}

Attacker can call get_balance with answerID = tvm.functionId(VulnerableContract.transfer) and throw exception in callback. In this case VulnerableContract will receive unexpected onBounce and will double their tokens.

To solve this problems my suggestion is to force users to set ALL three params explicitly. Just throw compile time error if one of (value, flag, bounce) is not set.

IgorKoval commented 2 years ago

Thank you! Yes, it's good hint!

mnill commented 1 year ago

Resolved in 0.60.0