Advanced Solidity: Avoid Relying On Contract Balance

Why having your smart contract flow rely on its balance is a severe security breach

A simple bidding contract

Let's assume you are developing a smart contract that?needs to perform an action when the total amount of Ether pushed into the contract reaches a certain threshold. Maybe you allow bidding for an NFT so that the bidding policy is that each bidder may bid a certain fixed amount and when the sum total of this amount reaches a threshold the NFT's ownership will be assigned to the last bidder, the contract funds will be transmitted to the contract owner, and the contract will reset itself for a new bid cycle.

While this approach is definitely non-standard for bidding, it will provide us with a simple and easy to grasp representation of a contract reliance on total-balance value, which is a rather common smart contract pattern.

Now, to implement such a last-win-all bidding mechanism you could write a function like bidForAsset below where which the user pushes his funds and, if the total amount of pushed funds is met, the NFT asset is transferred to the last bidder:

contract BiddingContract {


? uint constant public TOTAL_BIDDING_SUM = 120 ether;

??uint constant public BID_SUM = 1 ether;



??error MaxBidValueExceededError();

??error BiddingNotInProgressError();

??error TransferByNonOwnerError(address sender, uint value);



??bool public isInProgress = true;

??IERC721 public nftAddress;

??uint public nftTokenId;



??function bidForAsset() external payable {

????if (!isInProgress)

??????revert BiddingNotInProgressError();



????if (msg.value != BID_SUM)

??????revert MaxBidValueExceededError();



????uint contractBalance = address(this).balance;

????if (contractBalance >= TOTAL_BIDDING_SUM) {

??????isInProgress = false;


??????// transfer NFT to msg.sender

??????nftAddress.safeTransferFrom(nftOwner(), msg.sender, nftTokenId);


??????// transfer all funds to owner

??????require(owner().call{ value: contractBalance }(""));

????}

??}
}
        

Note that this function must be payable else any attempt to pass Ether into it will result in an error. Also note that I am using the current contract balance without increasing it with the msg.value passed into the function. The value of this.balance is always increased by the current msg.value by the EVM before the body of the current payable method is executed. Appending msg.value to the balance by code is in fact a rather common source of error.

Adding direct funding transfer route

So far so good. Now let's expand our requirements a bit and assume the contract also needs to allow Ether to be added into the contract by the owner in order e.g. to manage his exchange value, enhance its visibility or similar needs.

To allow that we can just define a receive function as payable. This will allow the owner to transfer any required funds:

   receive() external payable {}

        

There is no need for a function body here. Simply declaring a payable receive() is enough. You can achieve a similar effect by declaring the fallback() function as payable but receive() documents our intention in a clearer manner, so we'll stick to it.

By adding the above into our contract we have allowed users to join in and bid, while also allowing the owner to push funds inside. But we have also made our contract vulnerable to a rather nasty type of attack that will now describe.

Why is this a bad approach?

Because anyone can now circumvent the last-win-all logic by passing direct Ether deposits into the contract just before issuing their bid in the amount that will make sure they become the winners which, depending on the BID_SUM and NFT's market value, may be highly appealing for some.

Clearly, we have a problem. But the solution appears straightforward. All we need to do is to change the receive() function so as to only accept funds passed by the contract owner and throw an error in any other case.

That is not too hard:

??receive() external payable { 

???  if (msg.sender != owner())
 
?   ????revert TransferByNonOwnerError(msg.sender, msg.value);

??}

        

And we're on safe ground again. Or are we?

Introducing selfDestruct

A somewhat less used Solidity function allows a contract - any contract - to be altogether removed from the blockchain, effectively rendering the contract as non-existing. This function is aptly named selfDestruct() and, once invoked, besides removing the contract it will also transfer any remaining Ether left inside the contract to an address provided by the caller. As is always the rule in Solidity, this address may belong to a human (EOA in Solidity docs) or to a smart contract.

And now comes the tricky part - since the designers of selfDestruct() realized that it is a 'one shot' sort of service after which there will no longer be a contract to interact with, it was decided that the Ether transfer performed by this function will not go through the forwarded contract's receive() or fallback() function, as those have the potential of failing. In fact, the transfer operation performed by selfDestruct() circumvents any of the destination contract's logic and basically just lands the funds directly into the destination contract's balance.

This behavior is unique to selfDestruct(). All other Ether deposit functions - call, send, transfer - will always end up invoking the target contract's receive or fallback methods. And it is this uniqueness of selfDestruct() that will allow a bad player to work around our contract's protection and break it again.

Introducing a malicious contract

To do that, the attacker may deploy a malicious contract which we have creatively called BadPlayer, pass into it the required funds needed to forcefully win the bid and, after that, basically lurk in the dark waiting for the right moment.

contract BadPlayer {

??address biddingContractAddress;


??constructor(address _biddingContractAddress) {

???? biddingContractAddress = _biddingContractAddress;

??}



??function injectEthIntoBiddingContract() external {

??? this.selfDestruct(payable(biddingContractAddress));

??}


??receive() external payable {}

}

        

And, when the moment comes, the bad player will invoke injectEthIntoBiddingContract() just before performing his own bid. The function will kill the bad contract, which is the side-effect here, while also transferring all the Ether locked within the bad contract into the bidding contract without invoking any of the latter's protective logic!

Suppose the bad player is 3 Ether short of winning the bid, all he has to do is to transfer 2 Ethers to the bad contract, invoke the force-feeding function, and then perform his 1 Ether bidding into the bidding contract with assured success.

Fixing the root problem

The point to be taken from this discussion, and it holds even if you're not planning on writing a bidding contract anytime soon, is that a contract balance is very easy to manipulate by a rogue player, and hence should never be used in the major flow of the contract.

Once this is clear, the fix is straightforward: create an accumulatedFunds counter which counts only the funds that were legitimately transferred by playing users and use it to manage the contract's flow:

uint accumulatedFunds;


function bidForAsset_Fixed() external payable {

??if (!isInProgress)

????revert BiddingNotInProgressError();


??if (msg.value != BID_SUM)

????revert MaxBidValueExceededError();


??accumulatedFunds += msg.value;


??if (accumulatedFunds >= TOTAL_BIDDING_SUM) {

????isInProgress = false;

????accumulatedFunds = 0;


????// transfer nft to msg.sender

????nftAddress.safeTransferFrom(nftOwner(), msg.sender, nftTokenId);


????// transfer all funds to owner

????require(owner().call{ value: address(this).balance }(""));

??}

}

        

Final thoughts

All this should not be taken the wrong way, the balance field is not your enemy and you may safely use it, as we have done above, when extracting funds from the contract or when querying its status but, again, never to actually determine the contract's flow. This is in fact one example of a larger concept that is probably the only way for writing truly secure Solidity contracts:

You must never assume good behavior from the contracts you are interacting with. In fact, you should always assume they will do their utmost to break your flow and program your code accordingly.