RACE #18 Of The Secureum Bootcamp Epoch∞
This is a Write-Up of RACE-18, Quiz of the Secureum Bootcamp (opens in a new tab) for Ethereum Smart Contract Auditors. It was designed by Secureum Mentor Josselin Feist (opens in a new tab) from Trail of Bits (opens in a new tab).
Participants of this quiz had to answer 8 questions within the strict time limit of 16 minutes. If you’re reading this in preparation for participating yourself, it’s best to give it a try under the same time limit!
As usual, I waited for submissions to close before publishing it and, to stay true to the original, I omitted syntax highlighting. Feel free to copy it into your favorite editor, but do so after starting the timer!
May 25, 2023 by patrickd
Code
All 8 questions in this RACE are based on the below contract. This is the same contract you will see for all the 8 questions in this RACE. The question is below the shown contract.
pragma solidity ^0.8.0; // using 0.8 to be safe from arithmetic risks
interface IERC20 {
function decimals() external view returns (uint256);
function balanceOf(address account) external view returns (uint256);
function transfer(address to, uint256 amount) external returns (bool);
function allowance(address owner, address spender) external view returns (uint256);
function approve(address spender, uint256 amount) external returns (bool);
function transferFrom(address from, address to, uint256 amount) external returns (bool);
function totalSupply() external view returns (uint256);
// Mint tokens
// The access controls only allows the Bank contract to call this function
// mint does not have callback (i.e. it does not allow reentrancies)
// [Secureum] Assume no bug
function mint(uint amount, address to) external;
// Burn token
// The access controls only allows the Bank contract to call this function
// burn does not have callback (i.e. it does not allow reentrancies)
// [Secureum] Assume no bug
function burn(uint amount, address to) external;
}
// This contract allows to deposit ether and mint tokens by locking Ether, at a rate of 1 ether = 10 tokens
// The user needs to burn the token to unlock their balance
// Once they minted tokens, the user cannot mint more, unless they burned the original amount
// This contract is safe from reentrancies and front running risks
contract Bank{
mapping(address => uint) balances;
mapping(address => uint) minted;
uint decimals_factor = 10**18;
IERC20 token;
uint8 isReenter = 1;
modifier nonReentrant{
require(isReenter>=1);
isReenter = 2;
_;
isReenter = 1;
}
constructor(address token_address){
token = IERC20(token_address);
// Only accept tokens with 18 decimals to ease the math
require(token.decimals() == 18);
}
/***
Deposit function
***/
function deposit(uint amount) internal{
balances[msg.sender] += amount;
}
function deposit() payable nonReentrant public{
deposit(msg.value);
}
function depositTo(address to) payable nonReentrant public{
balances[to] = msg.value;
}
/***
Getter
***/
// Return the balance of the user
function getBalances(address user) public nonReentrant returns(uint balance){
uint balance = balances[user];
}
/***
Withdraw functions
***/
function withdraw(uint amount) nonReentrant public{
// Cannot withdraw if some tokens have been minted
require(minted[msg.sender]==0);
msg.sender.call{value: amount}("");
balances[msg.sender] -= amount;
}
function withdrawAll() nonReentrant public{
// Cannot withdraw if some tokens have been minted
require(minted[msg.sender]==0);
balances[msg.sender] = 0;
msg.sender.call{value: balances[msg.sender]}("");
}
/***
Mint/burn functions
***/
// Mint tokens
// The user must have locked enough ether in the contract
// This function only mint whole number and not fraction.
// As a result, amount is expressed without the decimals.
// To mint 10 tokens, call mint(10);
// @param amount The number of tokens to be minted. amount is the whole number.
function mint(uint amount) nonReentrant public{
// Cannot mint if some tokens have already been minted
require(minted[msg.sender]==0);
_has_enough_balance(amount, balances[msg.sender]);
minted[msg.sender] = amount * decimals_factor;
// [Secureum] Assume token.mint has no bug and no callback
token.mint(amount * decimals_factor, msg.sender);
}
/// @notice burn the token and unlock the account.
function burn() nonReentrant public{
// This will revert if the user does not have minted[msg.sender] tokens in its balance
require(token.balanceOf(msg.sender) == minted[msg.sender]);
// [Secureum] Assume token.burn has no bug and no callback
token.burn(minted[msg.sender], msg.sender);
minted[msg.sender] = 0;
}
/// @notice Ensure that enough ethers are locked. 1 ether allows to mint 10 tokens
/// @param desired_tokens The number of tokens to buy
/// @param balance The ether balance available
function _has_enough_balance(uint256 desired_tokens, uint256 balance) internal view {
uint256 required_balance = (desired_tokens / 10) * decimals_factor;
require(balance >= required_balance);
}
}
Question 1 of 8
What risk(s) should be considered when reviewing this contract?
- A. Reentrancy risks
- B. Logic bugs
- C. Front-running risks
- D. Arithmetic risks
Solution
Correct is A, B, C, D.
During an audit all risks should be considered! (Duh)
This question wasn't about what issues the contract actually has, but these specific issues will be asked about later in this quiz.
Question 2 of 8
Which of the following statement(s) is/are true?
- A. No overflow can ever occur in a contract compiled with solc version 0.8
- B.
IERC20.decimals returns(uint256)
is not a correct ERC20 function according to the ERC20 specification - C. The contract does not follow Natspec for all the documentation
- D. None of the above
Solution
Correct is B, C.
- A: With Solidity version 0.8, overflows (eg. from addition or multiplication) can still happen within
unsafe
blocks. - B: According to the ERC20 specification, the
decimals()
function should returnuint8
. - C: It's not followed everywhere, eg.: The
mint()
function's@param
is not following Natspec because it's missing one slash.
Question 3 of 8
Which of the following is an/are invariant(s) that should hold true? (assuming no bug)
- A. The contract's ether balance must be strictly equal to the sum of all the balances (in the
balances
mapping) - B. For any user,
minted[user] <= balances[user] * 10
- C. For any user,
token.balanceOf(user) == balances[user]
- D. None of the above
Solution
Correct is B.
- A: Strict equality with a contract's ether balance is an invariant that would be broken by an injection of ether value into the contract (eg. via
SELFDESTRUCT
) - B: This one will hold. A user is able to mint 10 times as many tokens as their ether balance. But they might have a larger balance than actually minted tokens. And most importantly, they should never have more minted token than what they have locked.
- C: A user could transfer their token balance breaking the invariant.
Question 4 of 8
Which of the following sentence(s) is/are true regarding getBalances
?
- A.
getBalances(msg.sender)
returns the sender's balance - B.
getBalances
reverts if the user's balance is zero - C.
getBalances
always returns zero - D. None of the above
Solution
Correct is C.
While the function never reverts, it doesn't actually return the sender's balance. That's because the balance
return variable is shadowed within the function's body (ie. a new variable with the same name is declared and used instead of the correct one). As the default value of unused variables is always zero-like the number returned by this function will always be 0 too.
Question 5 of 8
Which of the following sentence(s) is/are true regarding the balances
mapping?
- A. An attacker can increase their
balances
(theft) frombalances[victim]
- B. An attacker can reset
balances[victim]
- C. An attacker can increase their
balances
to any amount - D. An attacker cannot compromise the
balances
mapping
Solution
Correct is B.
There are no vectors for A and C.
But the depositTo()
function is mistakenly using =
instead of =+
, therefore resetting the destination's balance. Attacker's can use this to set the balances[victim]
to arbitrary values, most likely very small ones.
Question 6 of 8
Which of the following sentence(s) is/are true regarding reentrancies in this contract?
- A.
nonReentrant
protects the contract from reentrancies - B. A correct reentrancy protection modifier is not needed if
withdraw
is refactored to follow the CEI pattern - C. There are no reentrancy risks
- D. None of the above
Solution
Correct is B.
- A: The
nonReentrant
modifier is not correctly implemented and won't protect the contract from reentrancies. (Its require should check for strict equality) - B: That's correct, following the CEI pattern would mean updating the user's balance before calling the
msg.sender
, making reentering the contract pointless. - C: The
withdraw
function does not follow the CEI pattern at this moment and the reentrancy-guard is broken, therefore there is a reentrancy risk.
Question 7 of 8
The mint
function has the following risks (assuming there are no bugs in the other functions)
- A. The user can generate tokens without having locked ether
- B. An attacker can front-run a call to
mint
and make it revert - C.
minted[msg.sender] = amount * decimals_factor;
should be replaced byminted[msg.sender] = amount / decimals_factor;
- D. None of the above
Solution
Correct is A.
- A: Is possible by exploiting a loss of precision error due to division-before-multiplication in
_has_enough_balance()
. This effectively allows minting up to 9 tokens without locking any ether. - B: There's nothing that a frontrunner could do to make a call to
mint
revert. - C: That change would not make sense.
Question 8 of 8
The burn
and _has_enough_balance
functions have the following risks (assuming there are no bugs in the other functions)
- A. The user can unlock their balance without burning the underlying tokens
- B. An attacker can front-run a call to
burn
and make it revert - C.
burn
should usetx.origin
instead ofmsg.sender
to prevent access control issues - D. None of the above
Solution
Correct is B.
- A: No way to do this.
- B: Due to the strict equality requirement in
burn
, a frontrunner can transfer 1 wei of token to the user to make the burn-call revert. - C: Making this change would likely make the contract less secure (opens phishing vector) and would also be bad for composability (only EOAs could be users).