Assignment - SafeMath

JJ_FX · October 26, 2021, 12:51pm

Hi! So I’ve been using my own Bank contract. Now the contract is much cleaner and is more secure - big thanks @jon_m! Here’s the SafeMath in my Bank contract:

// SPDX-License-Identifier: UNLICENCED
pragma solidity 0.8.9;
import "./Academy-Ownable.sol";
import "./SafeMath.sol";

contract Bank is Ownable {
    
    using SafeMath for uint;
    
    mapping(address => Customer) customers;
    address[] customerLog;
    
    function registerAccount() public payable {
        require(!customers[msg.sender].registered);
        customers[msg.sender] = Customer(msg.value, true);
        customerLog.push(msg.sender);
    }
    
    function deposit() public payable mRegistered {
        customers[msg.sender].balance = customers[msg.sender].balance.add(msg.value);
    }
    
    function getBalance() public view mRegistered returns (uint) {
        return customers[msg.sender].balance;
    }
    
    function transfer(address _to, uint _amount) public mRegistered {
        require(customers[msg.sender].balance >= _amount, "Insuffient balance!");
        payable(_to).transfer(_amount);
        customers[msg.sender].balance = customers[msg.sender].balance.sub(_amount);
    }
    
    function withdraw(uint _amount) public {
        transfer(msg.sender, _amount);
    }

    function closeBank() public mOwnerOnly {
        for (uint i=0; i<customerLog.length; i++) {
            if (customers[customerLog[i]].balance > 0) {
                payable(customerLog[i]).transfer(customers[customerLog[i]].balance);
            }
        }
        
        selfdestruct(payable(owner));
    }
    
    modifier mRegistered {
        require(customers[msg.sender].registered, "You are not registered!");
        _;
    }

}

Academy-Ownable.sol

// SPDX-License-Identifier: UNLICENCED
pragma solidity 0.8.9;

contract Ownable {
    
    address public owner;
    
    struct Customer {
        uint balance;
        bool registered;
    }
    
    constructor() {
        owner = msg.sender;
    }
    
    modifier mOwnerOnly {
        require(msg.sender == owner, "You are not the owner");
        _;
    }
}

thecil · October 26, 2021, 6:48pm

Indeed, it does work on asserts also

Carlos Z

jon_m · October 27, 2021, 9:18pm

Very nicely done @JJ_FX

Just a few final comments …

(1) As the Customer struct specifically relates to Bank contract customers, this would be better defined in Bank. One of the key advantages of inheritance is being able to re-use modules of code and, in so doing, avoid code duplication. Ownable becomes more reusable, and also more meaningful as a separate module, if it only contains functionality associated with contract ownership.

(2) I would include the mRegistered modifier in the withdraw() function header as well. Even though this constraint will be applied by the same modifier when the transfer() function is called from withdraw(), we still want to trigger revert as early as possible, because this will reduce the number of operations performed before revert is triggered and will therefore maximise the amount of gas refunded.

(3) Don’t forget that in your transfer() function it’s important to modify the contract state for the reduction in the individual user’s balance…

before actually transferring the funds out of the contract to the external address…

… just in case there is an attack after the transfer, but before the state is modified to reflect this operation. If you haven’t already, you’ll learn about the type of attack this prevents, and how it does it later on.

Where possible, applying the following order to the statements in the function body helps to reduce security risk:

check inputs (require statements)
effects (update the contract state)
external interactions

Michael_Gerst · October 28, 2021, 12:41pm

Hello,

Here is the code:

pragma solidity ^0.8.0;
pragma abicoder v2;
import "./Ownable.sol";
import "./SafeMath.sol";

contract Bank is Ownable {
    using SafeMath for uint256;
    
    mapping(address => uint) balance;
    address[] customers;
    
    event depositDone(uint amount, address indexed depositedTo);
    
    function deposit() public payable returns (uint)  {
        balance[msg.sender] += msg.value;
        emit depositDone(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    function withdraw(uint amount) public onlyOwner returns (uint){
        require(balance[msg.sender] >= amount);
        balance[msg.sender] -= amount;
        payable(msg.sender).transfer(amount);
        return balance[msg.sender];
    }
    
    function getBalance() public view returns (uint){
        return balance[msg.sender];
    }
    
    function transfer(address recipient, uint amount) public {
        require(balance[msg.sender] >= amount, "Balance not sufficient");
        require(msg.sender != recipient, "Don't transfer money to yourself");
        
        uint previousSenderBalance = balance[msg.sender];
        
        _transfer(msg.sender, recipient, amount);
        
        assert(balance[msg.sender] == previousSenderBalance - amount);
    }
    
    function _transfer(address from, address to, uint amount) private {
        balance[from] -= amount;
        balance[to] += amount;
    }
    
}

Shadowstep2003 · October 31, 2021, 11:57pm

My code is below, but I’m pretty confused on the call to the SafeMath functions.

In my deposit function I kept my original code before taking a peek to see if I was on the right track. Therefore, all other SafeMath calls in other functions are more in line with the official solution posted.

For the below

balance[msg.sender] = SafeMath.add(balance[msg.sender], msg.value);

I can clearly see that the add function in SafeMath takes two parameters. Why do we not call the Safemath function with the two parameters in parentheses? I included SafeMath before the .add to imply that we are using the add function from the SafeMath library, but perhaps this is incorrect and it should instead be simply:

add(balance[msg.sender], msg.value);

If you told me the above was also incorrect I would point to the transfer call in function transfer below which follows similar format to the above

_transfer(msg.sender, recipient, amount);

Any clarification on the above points would greatly appreciated. It also leads me to wonder what happens if I declare similar function names in my Contract to the external library SafeMath. For example, I tried quickly adding an ‘add’ function in the Bank contract and it actually compiled which makes me wonder how the Bank Contract knows what function to use at all

pragma solidity 0.8.0;
pragma abicoder v2;
import "./artifacts/Ownable.sol";
import "./Safemath.sol";

contract Bank is Ownable {
    
    using SafeMath for uint;
    
    mapping(address => uint) balance;
    address[] customers;
    
    event depositDone(uint amount, address indexed depositedTo);
    
    function deposit() public payable returns (uint)  {
        //balance[msg.sender] += msg.value;
        balance[msg.sender] = SafeMath.add(balance[msg.sender], msg.value);
        emit depositDone(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    function withdraw(uint amount) public onlyOwner returns (uint){
        require(balance[msg.sender] >= amount);
        //balance[msg.sender] -= amount;
        balance[msg.sender] = balance[msg.sender].sub(amount);
        payable(msg.sender).transfer(amount);
        return balance[msg.sender];
    }
    
    function getBalance() public view returns (uint){
        return balance[msg.sender];
    }
    
    function transfer(address recipient, uint amount) public {
        require(balance[msg.sender] >= amount, "Balance not sufficient");
        require(msg.sender != recipient, "Don't transfer money to yourself");
        
        uint previousSenderBalance = balance[msg.sender];
        
        _transfer(msg.sender, recipient, amount); 
        
        assert(balance[msg.sender] == previousSenderBalance - amount);
    }
    
    function _transfer(address from, address to, uint amount) private {
        balance[from] = balance[from].sub(amount);
        balance[to] = balance[to].add(amount);
    }
    
    
}

thecil · November 1, 2021, 10:13am

Hey @Shadowstep2003, hope you are great.

The difference comes from the visibility level of the safemath functions and just creating another one with the same name, SafeMath functions are internal, which means that can only be called internally in the contract, pure also means that it will not modify any statement of the contract, rather than return an external operation based on a input data without any direct impact on the blockchain data.

Carlos Z

Lennart_Lucas · November 4, 2021, 12:42pm

pragma solidity 0.7.5;
pragma abicoder v2;
import "./Ownable.sol";
import "./Safemath.sol";

contract Bank is Ownable {
    
    mapping(address => uint) balance;
    address[] customers;
    
    event depositDone(uint amount, address indexed depositedTo);
    
    function deposit() public payable returns (uint)  {
        balance[msg.sender] = balance[msg.sender].add(msg.value);
        emit depositDone(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    function withdraw(uint amount) public onlyOwner returns (uint){
        require(balance[msg.sender] >= amount);
        balance[msg.sender] = balance[msg.sender].sub(amount);
        payable(msg.sender).transfer(amount);
        return balance[msg.sender];
    }
    
    function getBalance() public view returns (uint){
        return balance[msg.sender];
    }
    
    function transfer(address recipient, uint amount) public {
        require(balance[msg.sender] >= amount, "Balance not sufficient");
        require(msg.sender != recipient, "Don't transfer money to yourself");
        
        uint previousSenderBalance = balance[msg.sender];
        _transfer(msg.sender, recipient, amount);
        
        assert(balance[msg.sender] == previousSenderBalance - amount);
    }
    
    function _transfer(address from, address to, uint amount) private {
        balance[from] = balance[from].sub(amount);
        balance[to] = balance[to].add(amount);
    }
    
}

MightyYak · November 6, 2021, 1:23am

I’ve got a question - how come we only need to pass 1 argument into add() / sub() from SafeMath? is it similar to the functionality of += and -=?

pragma solidity 0.8.0;
pragma abicoder v2;
import "./Ownable.sol";
import "./safeMath.sol";

contract Bank is Ownable {
    
    using SafeMath for uint256;
    
    mapping(address => uint) balance;
    address[] customers;
    
    event depositDone(uint amount, address indexed depositedTo);
    
    function deposit() public payable returns (uint)  {
        balance[msg.sender] = balance[msg.sender].add(msg.value);
        emit depositDone(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    function withdraw(uint amount) public onlyOwner returns (uint){
        require(balance[msg.sender] >= amount);
        balance[msg.sender] = balance[msg.sender].sub(amount);
        payable(msg.sender).transfer(amount);
        return balance[msg.sender];
    }
    
    function getBalance() public view returns (uint){
        return balance[msg.sender];
    }
    
    function transfer(address recipient, uint amount) public {
        require(balance[msg.sender] >= amount, "Balance not sufficient");
        require(msg.sender != recipient, "Don't transfer money to yourself");
        
        uint previousSenderBalance = balance[msg.sender];
        
        _transfer(msg.sender, recipient, amount);
        
        assert(balance[msg.sender] == previousSenderBalance.sub(amount));
    }
    
    function _transfer(address from, address to, uint amount) private {
        balance[from] = balance[from].sub(amount);
        balance[to] = balance[to].add(amount);
    }
    
}

Ruben99 · November 11, 2021, 12:08pm

Here’s my solution to the assigment. Just changed all the places where there was any kind of addition or substraction using SafeMath.

pragma solidity 0.8.0;
pragma abicoder v2;
import "./SafeMath.sol";
import "./Ownable.sol";

contract Bank is Ownable{
    
    using SafeMath for uint256;
    
    mapping(address => uint) balance;
    address[] customers;
    
    event depositDone(uint amount, address indexed depositedTo);
    
    function deposit() public payable returns (uint)  {
        //balance[msg.sender] += msg.value;
        balance[msg.sender] = balance[msg.sender].add(msg.value);
        emit depositDone(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    function withdraw(uint amount) public onlyOwner returns (uint){
        require(balance[msg.sender] >= amount);
        //balance[msg.sender] -= amount;
        balance[msg.sender] = balance[msg.sender].sub(amount);
        payable(msg.sender).transfer(amount); //esta no es la forma más óptima de enviar fondos dado que podemos utilizar un ataque de fallback
        return balance[msg.sender];
    }
    
    function getBalance() public view returns (uint){
        return balance[msg.sender];
    }
    
    function transfer(address recipient, uint amount) public {
        require(balance[msg.sender] >= amount, "Balance not sufficient");
        require(msg.sender != recipient, "Don't transfer money to yourself");
        
        uint previousSenderBalance = balance[msg.sender];
        
        _transfer(msg.sender, recipient, amount);
        
        assert(balance[msg.sender] == previousSenderBalance - amount);
    }
    
    function _transfer(address from, address to, uint amount) private {
        //balance[from] -= amount;
        balance[from] = balance[from].sub(amount);
        //balance[to] += amount;
        balance[to] = balance[to].add(amount);
    }
    
}

serba · November 13, 2021, 4:19pm

So, I’m here doing the course again and discovered that I submitted this assignment incomplete.
Here is my code:

assignment-SafeMath

`
pragma solidity 0.8.0;
pragma abicoder v2;
import “./Ownable.sol”;
import “./safemath.sol”;

contract Bank is Ownable {

using SafeMath for uint256;

mapping(address => uint256) balance;
address[] customers;

event depositDone(uint256 amount, address indexed depositedTo);

function deposit() public payable returns (uint256)  {
    balance[msg.sender] = balance[msg.sender].add(msg.value);
    emit depositDone(msg.value, msg.sender);
    return balance[msg.sender];
}

function withdraw(uint256 amount) public onlyOwner returns (uint256){
    require(balance[msg.sender] >= amount);
    balance[msg.sender] = balance[msg.sender].sub(amount);
    payable(msg.sender).transfer(amount);
    return balance[msg.sender];
}

function getBalance() public view returns (uint256){
    return balance[msg.sender];
}

function transfer(address recipient, uint256 amount) public {
    require(balance[msg.sender] >= amount, "Balance not sufficient");
    require(msg.sender != recipient, "Don't transfer money to yourself");
    
    uint256 previousSenderBalance = balance[msg.sender];
    
    _transfer(msg.sender, recipient, amount);
    
    assert(balance[msg.sender] == previousSenderBalance - amount);
}

function _transfer(address from, address to, uint256 amount) private {
    balance[from] = balance[from].sub(amount);
    balance[to] = balance[to].add(amount);
}

}
`

Gry · November 15, 2021, 5:40pm

pragma solidity 0.8.0;
pragma abicoder v2;
import "./Ownable.sol";
import "./safemath.sol";

contract Bank is Ownable {
    
    using SafeMath for uint256;
    
    mapping(address => uint) balance;
    address[] customers;
    
    event depositDone(uint amount, address indexed depositedTo);
    
    function deposit() public payable returns (uint)  {
        balance[msg.sender] += msg.value;
        emit depositDone(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    function withdraw(uint amount) public onlyOwner returns (uint){
        require(balance[msg.sender] >= amount);
        balance[msg.sender] -= amount;
        payable(msg.sender).transfer(amount);
        return balance[msg.sender];
    }
    
    function getBalance() public view returns (uint){
        return balance[msg.sender];
    }
    
    function transfer(address recipient, uint amount) public {
        require(balance[msg.sender] >= amount, "Balance not sufficient");
        require(msg.sender != recipient, "Don't transfer money to yourself");
        
        uint previousSenderBalance = balance[msg.sender];
        
        _transfer(msg.sender, recipient, amount);
        
        assert(balance[msg.sender] == previousSenderBalance - amount);
    }
    
    function _transfer(address from, address to, uint amount) private {
        balance[from] -= amount;
        balance[to] += amount;
    }
    
}

thecil · November 17, 2021, 6:33pm

Hey @Gry, hope you are well.

Although you have imported the SafeMath library properly into the contract and you are using it for uint256, you forgot to change the syntax in the functions, take a look on the other replies above our replies to have an idea on what to do

Carlos Z

Gry · November 18, 2021, 12:10pm

@thecil
Morning,

I first had the uint´s changed to uint256s in the functions but I changed them back to uint once I remembered that Filip said something along the lines that the “uint” is using 256 bit system by default, which would make the safemath implemented to 256 automatically also apply to plain “uint” syntaxes. Over- and -underflow protection should be implemented by default in the core because of this. Or I might have understood something wrong?

Jani

Petr_Krulis · November 18, 2021, 3:50pm

// SPDX-License-Identifier: GPL-3.0
pragma solidity 0.8.0;
pragma abicoder v2;
import “./SafeMath.sol”;

contract Bank is Ownable {

using SafeMath for uint256;

Astro · November 20, 2021, 12:16pm

pragma solidity 0.8.0;
pragma abicoder v2;
import "./Ownable.sol";
import "./safemath.sol";

contract Bank is Ownable {
    
    mapping(address => uint) balance;
    address[] customers;
    
    event depositDone(uint amount, address indexed depositedTo);
    
    function deposit() public payable returns (uint)  {
        balance[msg.sender] = balance[msg.sender].add(msg.value);
        emit depositDone(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    function withdraw(uint amount) public onlyOwner returns (uint){
        require(balance[msg.sender] >= amount);
        balance[msg.sender] = balance[msg.sender].sub(amount);
        payable(msg.sender).transfer(amount);
        return balance[msg.sender];
    }
    
    function getBalance() public view returns (uint){
        return balance[msg.sender];
    }
    
    function transfer(address recipient, uint amount) public {
        require(balance[msg.sender] >= amount, "Balance not sufficient");
        require(msg.sender != recipient, "Don't transfer money to yourself");
        
        uint previousSenderBalance = balance[msg.sender];
        
        _transfer(msg.sender, recipient, amount);
        
        assert(balance[msg.sender] == previousSenderBalance - amount);
    }
    
    function _transfer(address from, address to, uint amount) private {
        balance[from] = balance[from].sub(amount);
        balance[to] = balance[to].add(amount);
    }
    
}

Cero · November 21, 2021, 1:58pm

Hello.
I did the same as Filip but get all the time this error in 15. line.
Does anybody know why?

JJ_FX · November 22, 2021, 10:19am

Hi @Cero

You need to tell solidity to use SafeMath after the import. So just add:

using SafeMath for uint;

You’re also using ^0.8.x compiler, so you might as well just stay away from SafeMath. As in 0.8 the operations on underflow and overflow has been fixed and they revert on default.

So in other words the regular math in ^0.8.x like x + y is protected against underflow/overflaw. I’m quite surprised that the course is still using SafeMath everywhere. @filip Maybe you should update the course because soon SafeMath will become an outdated solution?

You can read more in the docs:
https://docs.soliditylang.org/en/v0.8.10/080-breaking-changes.html

Cheers

Talk2coded · November 22, 2021, 1:41pm

pragma solidity 0.5.0;

import "./Ownable.sol";
import "safemath.sol";

contract Bank is Ownable {
    
    using safemath for uint256;
    
    mapping(address => uint256) balance;
    address[] customers;
    
    event depositDone(uint256 amount, address indexed depositedTo);
    
    function deposit() public payable returns (uint256)  {
        balance[msg.sender] += msg.value;
        emit depositDone(msg.value, msg.sender);
        return balance[msg.sender];
    }
    
    function withdraw(uint256 amount) public onlyOwner returns (uint256){
        require(balance[msg.sender] >= amount);
        balance[msg.sender] -= amount;
        payable(msg.sender).transfer(amount);
        return balance[msg.sender];
    }
    
    function getBalance() public view returns (uint256){
        return balance[msg.sender];
    }
    
    function transfer(address recipient, uint256 amount) public {
        require(balance[msg.sender] >= amount, "Balance not sufficient");
        require(msg.sender != recipient, "Don't transfer money to yourself");
        
        uint256 previousSenderBalance = balance[msg.sender];
        
        _transfer(msg.sender, recipient, amount);
        
        assert(balance[msg.sender] == previousSenderBalance - amount);
    }
    
    function _transfer(address from, address to, uint256 amount) private {
        balance[from] -= amount;
        balance[to] += amount;
    }
    
}

JJ_FX · November 22, 2021, 12:13pm

Hey @Talk2coded,

So the whole point of this exercise is to use SafeMath. And you’ve imported it:

import "safemath.sol";

Then you add this, in order to be able to use it for uint256

using safemath for uint256;

But then in the end you don’t use any of the SafeMath at all. You’re using the regular math. Which is just fine, because you’re using ^0.8.x compiler - by default this version reverts on under/over flows.

So if you want to stay with ^0.8.x then just remove SafeMath completely. Or change to 0.7.x or lower (with SafeMath) and use uint256.add() or uint256.sub().

See my post just above. Safemath becomes irrelevant for compiler ^0.8.x.

EDIT: Also if you look at breaking changes for v0.8.0, the ABI coder v2 is activated by default. So as well you can remove this:

pragma abicoder v2;

https://docs.soliditylang.org/en/v0.8.10/080-breaking-changes.html

Cheers!

Talk2coded · November 22, 2021, 1:42pm

Okay, you’re basically saying that safemath is used on pragma versions less than 0.8.

Apart from adding “use safemath” i changed uint name I uint256. Does it have any effect ?