TVM-Solidity-Compiler icon indicating copy to clipboard operation
TVM-Solidity-Compiler copied to clipboard

Proposal: force users to explicitly specify responsible answer flags.

Open mnill opened this issue 3 years ago • 1 comments

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.

mnill avatar Feb 21 '22 12:02 mnill

Thank you! Yes, it's good hint!

IgorKoval avatar Feb 28 '22 14:02 IgorKoval

Resolved in 0.60.0

mnill avatar Nov 19 '22 02:11 mnill