Learn Solidity Security issues with this Solidity Audit Quiz

Last updated on December 2022. Created on November 2022 • Tags: ethereum auditing solidity

Test your knowledge on spotting Solidity security issues

This is a selection of example codes with security issues. Can you spot the security issues with them?

These are simplified examples, and the purpose of this page is to help learn about some simple Solidity security issues. I’ve also tried to format it so its readable on a mobile device.

Most of these are simplified enough that you can probably find a few mistakes (especially gas optimizations, missing typical events that would be emitted etc). If you are not sure what issue it is looking for, click the “hint” button and you should figure it out.

Ignore the following types of issues on this page:

gas optimizations (nothing on this page is optimized for gas! I’ve tried to make the examples easier to read)
using magic numbers/strings, instead of defining them as constants
Also pretend that everything is wrapped in a contract, the pragma mark is set etc - I sometimes left them out to make the code snippets smaller.

Instead focus just on security issues or other important issues/bugs that come up in audits.

Most of these are typical examples of things seen when smart contract audits are conducted.

Found an issue, or have a suggestion? let me know!

note: This only has a few items at the moment - i’m going to try and add a few more every week

Note: this security issue is *not* about who can vote, the hard coded 20 value, who can release funds, or any gas optimizations. And its not the weird logic to add a vote. Its a simplified example.

// allow anyone to add a vote
function vote(bool yesOrNo) external {
 voters.push(msg.sender);
 votes[msg.sender] = yesOrNo;
}

// only release funds if we have at least 20 'true' votes
function releaseFunds() external  {
 uint numVotesTrue = 0;
 for(uint i = 0; i < voters.length; i++) {
  if(votes[voters[i]]) {
    numVotesTrue++;
  }
 }

 if(numVotesTrue > 20) {
  destAddress.transfer(address(this).balance);
 }
}

What happens if lots of users add a vote (calling vote()), then someone calls releaseFunds()?

Note: this security issue is *not* about who can withdraw. Assume we're happy for anyone to call withdrawAllFunds

contract GiveMoneyOut {
  uint balance = 0;
  address payable destAddress;

  constructor(address payable _destAddress) payable  {
      destAddress = _destAddress;
  }

  receive() external payable {
      balance += msg.value;
  }

  function withdrawAllFunds() external {
    destAddress.transfer(balance);
    require(address(this).balance == 0, "balance should now be 0");
  }
}

What happens if someone tries to withdraw, and the require() check fails? Could we ever withdraw? How could we get into a state like that?

Note: this is an overly simplified example. We're looking for something specific that is missing from an upgradable contract. Note: it is not about missing functions.

contract UpgradedableContract is Initializable {
  address someAddressVar; // some vars related to this specific contract

  function initialize() initializer public {
    /* do some set up */
  }

  /* other functions here */
}

This was an overly simplied example, and might have been hard to see what you should have spotted...
But there was no storage gap for the upgradable contract.
This might lead to a storage slot collision when the contract is upgraded. Without this, an upgraded contract might overwrite the existing storage data/the base contract could not be upgraded and add new storage without potentially overwriting existing data from an inherited contract.
This tends to be a uint256 array of 50 items, named __gap
This is often treated as a medium rated issue in audits.
read more about storage gaps

contract UpgradedableContract is Initializable {
address someAddressVar;
uint256[50] __gap;

function initialize() initializer public {
/* do some set up */
}

/* other functions here */
}

Note: ignore the fact that anyone can call this function

contract SendMoney {
  address payable dest;
  constructor(address payable _dest) payable {
    dest = _dest;
  }
  function withdraw(uint256 amount) external {
      require(address(this).balance >= amount);
      dest.transfer(amount);
  }
}

payable(someAddress).transfer(amt) has a gas limit of 2300. When would that be a problem?

Note: this is about knowing a specific issue with approve() in (some) ERC20 tokens. It isn't related to who can call this, it isn't related to the amount we're approving. It requires some background information about that function - clicking the hint button might guide you to the answer...

function approveForToken(address token, address addrToApprove) internal {
  IERC20(token).approve(addrToApprove, type(uint256).max);
}

Check out something like USDT's smart contract (specifically at its approve function) to see why this will have issues. My guide on weird things to know about erc20 tokens might help too.

Some ERC20 tokens require setting the amount of approved to 0
This is to avoid a security issue.
If Alice allows Bob to transfer 200 tokens, she could call approve(bobsAddress, 200). Then Alice decides to lower it to 100. Bob could see this change in the mempool, and could therefore spend 200 just before she sets it to 100, then straight after that could spend 100 (total 300, when at most Alice set it as 200).
The best workaround/fix for this is to ensure when changing approved amount to always set it back to 0 first.
full write up / details here
Because of this issue, some tokens such as USDT implement this logic, so you have to ensure you approve 0 tokens first
For more info see this list of weird things to know about (some) ERC20 tokens

Suggested fix:

function approveForToken(address addrToApprove, address token) internal {
  IERC20(token).approve(addrToApprove, 0); // << add this line
  IERC20(token).approve(addrToApprove, type(uint256).max);
}

This is more of a low priority issue, but still an important issue to resolve. We're looking for something to do with the approval system, and this issue won't apply to all smart contracts due to business logic. Also, ignore the missing implementation for onlyOwner. All functions are shown except the _approve() function and logic related to onlyOwner (both are not relevant for this issue).

contract SomeApprovalSystem {
 // list of approved addresses that can run _approve() function
 mapping(address => bool) approvers;

 // let an admin add an approved address
 function addApprover(address newApprover) onlyOwner external {
     approvers[newApprover] = true;
 }

 // if msg.sender is approved, then let them approve someIdToApprove
 function approveSomething(uint someIdToApprove) external {
     require(approvers[msg.sender], "you must be on approval list");

     _approve(someIdToApprove); // not shown/not relevant
 }
}

This requires some background info on how Yul works...

contract YourContract {
    uint remainingEth;

    // This function does multiple transfers.
    // If they fail, it should carry on and return any unused eth back to sender
    // (the bug/issue is not found in this function)
    function acceptAndTransferEth() payable public {
        // (implementation details not shown - treat this function as pseudocode)

        // keep track of eth value
        remainingEth += msg.value;

        // this reduces remainingEth for each transfer to be done
        // if a transfer fails, carry on
        loopThroughSomeThingsAndTransferSomeEth();

        // if any balance remains in remainingEth (due to failed transfers),
        // then return it back to msg.sender
        _returnDust()
    }

    // (for the bug: look only at this function)
    function _returnDust() private {
     uint256 _remainingETH = remainingETH;
     assembly {
         if gt(_remainingETH, 0) {
             let isTransferSuccess := call(
                 gas(),
                 caller(),
                 selfbalance(),
                 0,
                 0,
                 0,
                 0
             )
         }
     }
    }
}

Only look at the _returnDust() function. There is an issue with it. What happens if loopThroughSomeThingsAndTransferSomeEth() has some transfers that fail (execution is expected to carry on). Could a call to this contract get additional ETH, if a previous transaction had this 'failed' transfer...?

Could you give yourself a higher balance? Note, we are not looking for over/underflow issues. We are also not looking for any checks that the balances[from] is at least amount. Just assume that part is implemented.

pragma solidity >=0.8.4;

contract TransferSomething {
    mapping (address=>uint) public balances;

    // not shown: some functions that can mint / add balance

    function transfer(address from, address to, uint amount) public {
        uint fromBalance = balances[from];
        uint toBalance = balances[to];

        balances[from] = fromBalance - amount;
        balances[to] = toBalance + amount;
    }

}

There are no checks for the params in transfer(). What sort of values could you give that might introduce a bug? Take note of the temporary variables...