Smart-contract audition.

[lychees]

Harberger-Taxes-Billboards Quck-Audit

AABoard.sol
——————
(1) setAdministrationRate() — does not impact tax income calculation
and _customAdministrationRate is not effectively used. Thus, both can be removed.

(2) Remove unnecessary checks for saved Gas usage

modifier adBoardExists(uint256 adId) {
        require(adId >= 0 && _exists(adId));
        _;
    }

==>

 modifier adBoardExists(uint256 adId) {
        require(_exists(adId));
        _;
    }

(3) In the likely situation when parentOwner == addresss(0), the collected copyrightFee will be permanently locked in the contract, and there is no way to withdraw.

Suggestion:

function _doCollectTax(uint256 adId, uint256 parentId, uint256 actualTax, uint256 totalPayback) internal {
       uint256 percent = (_customCopyrightRate[adId] == 0) ? DEFAULT_COPYRIGHT_RATE : _customCopyrightRate[adId];
       uint256 copyrightFee = actualTax.mul(percent).div(PERCENTAGE_BASE);
       uint256 administrationFee = actualTax.sub(copyrightFee);

       if (copyrightFee > 0) {
           address parentOwner = ownerOf(parentId);
           address payable payableParentOwner = address(uint160(parentOwner));
           if (payableParentOwner != address(0)) {
               payableParentOwner.transfer(copyrightFee);
           }
       }
       …
   }

==>

function _doCollectTax(uint256 adId, uint256 parentId, uint256 actualTax, uint256 totalPayback) internal {
        uint256 percent = (_customCopyrightRate[adId] == 0) ? DEFAULT_COPYRIGHT_RATE : _customCopyrightRate[adId];
        uint256 copyrightFee = actualTax.mul(percent).div(PERCENTAGE_BASE);
        uint256 administrationFee = actualTax.sub(copyrightFee);

        if (copyrightFee > 0) {
            address parentOwner = ownerOf(parentId);
            address payable payableParentOwner = address(uint160(parentOwner));
            if (payableParentOwner != address(0)) {
                payableParentOwner.transfer(copyrightFee);
            } else {
                taxIncome[adId] = taxIncome[adId].add(copyrightFee);
                allTaxIncome = allTaxIncome.add(copyrightFee);
            }
        }
        …
    }

(4) In the unlikely case of rounding issues, any minor mis-calculation of allTaxIncome could potentially lock all ETHs inside the contract.

    /**
     * @dev Withdraw all the tax income
     */
    function withdrawTaxIncome() public onlyPauser {
        require(allTaxIncome > 0);
        uint256 total = allTaxIncome;
        allTaxIncome = 0;
        administrator.transfer(total);
    }

==>

    /**
     * @dev Withdraw all the tax income
     */
    function withdrawTaxIncome(uint256 amount) public onlyPauser {
        require(allTaxIncome > 0);
        if (amount == 0) amount = allTaxIncome;
        if (amount <= allTaxIncome) {
          allTaxIncome = allTaxIncome.sub(amount);
          administrator.transfer(amount);
       }
    }

(5) Refactor withdrawDeposit for better encapsulation and readability

function withdrawDeposit(uint256 adId, uint256 amount) public adBoardExists(adId) {
       require(msg.sender == ownerOf(adId));
       require(amount > 0);

       uint256 collected = 0;
       uint256 owed = 0;
       (collected, owed) = _collectTax(adId);

       AdBoardData storage data = allAdBoards[adId];
       _doCollectTax(adId, data.parentId, collected, 0);

       if (owed <= 0) {
           if (amount > data.deposit) {
               amount = data.deposit;
           }
           data.deposit = data.deposit.sub(amount);
           if (data.deposit <= 0) {
               data.price = 0;
           }
           if (amount > 0) {
               msg.sender.transfer(amount);
           }
       }

       emit WithdrawDepositEvent(adId);
   }

==>

function withdrawDeposit(uint256 adId, uint256 amount) public adBoardExists(adId) {
        require(msg.sender == ownerOf(adId));
        require(amount > 0);

        uint256 collected = 0;
        uint256 owed = 0;
        (collected, owed) = _collectTax(adId);

        AdBoardData storage data = allAdBoards[adId];

        if (owed <= 0) {
            if (amount > data.deposit) {
                amount = data.deposit;
            }
            data.deposit = data.deposit.sub(amount);
            if (data.deposit <= 0) {
                data.price = 0;
            }
        } else {
            amount = 0
        }

        _doCollectTax(adId, data.parentId, collected, amount);

        emit WithdrawDepositEvent(adId);
    }

(6) Add necessary sanity checks for risk parameters

function setTaxRate(uint256 adId, uint256 rate) public onlyPauser adBoardExists(adId) {
        require(rate > 0);
        _customTaxRate[adId] = rate;
    }

==>

 function setTaxRate(uint256 adId, uint256 rate) public onlyPauser adBoardExists(adId) {
        require(rate > 0 &&  rate <= PERCENTAGE_BASE);
        _customTaxRate[adId] = rate;
    }

(7)

function setCopyrightRate(uint256 adId, uint256 rate) public onlyPauser adBoardExists(adId) {
        require(rate > 0);
       _customCopyrightRate[adId] = rate;
    }

==>

   function setCopyrightRate(uint256 adId, uint256 rate) public onlyPauser adBoardExists(adId) {
        require(rate > 0 &&  rate <= PERCENTAGE_BASE);
       _customCopyrightRate[adId] = rate;
    }

(8) Follow the best-practice checks-effects-interactions pattern:

8.1

function buyAdBoard(uint256 adId, uint256 price, string memory content) public payable adBoardExists(adId) {
 …
        _doCollectTax(adId, data.parentId, collected, totalPayback);

        data.deposit = msg.value.sub(data.price);
        data.price = (data.deposit <= 0) ? 0 : price;
        data.content = content;
        
        _transferAdBoard(currentOwner, msg.sender, adId);

        emit BuyEvent(adId);
    }

==>

    function buyAdBoard(uint256 adId, uint256 price, string memory content) public payable adBoardExists(adId) {
 …

        data.deposit = msg.value.sub(data.price);
        data.price = (data.deposit <= 0) ? 0 : price;
        data.content = content;
        
        _doCollectTax(adId, data.parentId, collected, totalPayback);
        _transferAdBoard(currentOwner, msg.sender, adId);

        emit BuyEvent(adId);
    }

8.2

 function addDeposit(uint256 adId) public payable adBoardExists(adId) {
 …
        AdBoardData storage data = allAdBoards[adId];
        _doCollectTax(adId, data.parentId, collected, 0);
        data.deposit = data.deposit.add(depositToAdd);
        if (data.deposit <= 0) {
            data.price = 0;
        }

        emit AddDepositEvent(adId);
    }

==>

    function addDeposit(uint256 adId) public payable adBoardExists(adId) {
 …
        AdBoardData storage data = allAdBoards[adId];
        data.deposit = data.deposit.add(depositToAdd);
        if (data.deposit <= 0) {
            data.price = 0;
        }

        _doCollectTax(adId, data.parentId, collected, 0);

        emit AddDepositEvent(adId);
    }

8.3 function withdrawDeposit(uint256 adId, uint256 amount) public adBoardExists(adId)

(9) Remove unnecessary checks for saved Gas usage.

9.1 buyAdBoard():

1. Line 164: require(msg.sender != address(0)) is redundant and can be removed
2. Line 165: require(price >= 0, "buyAdBoard: Price should be greater than or eaqual to 0") is redundant and can be removed
3. Line 168: require(currentOwner != address(0) && msg.sender != currentOwner) ==> require(msg.sender != currentOwner);

9.2 createAdBoard():

1. Line 212: require(msg.sender != address(0)) is redundant and can be removed
2. Line 213: require(parentId >= 0) is redundant and can be removed
3. Line 214: require(price >= 0, "createAdBoard: Price should be greater than or eaqual to 0") is redundant and can be removed

9.3 changePrice():

1. Line 269: require(price >= 0, "changePrice: Incorrect Price") is redundant and can be removed

9.4 _taxOwed():

1. Line 471: require(price >= 0 && lastTaxPayTimestamp > 0) ==> require(lastTaxPayTimestamp > 0);

(10) Enhanced event generation

event CreateEvent(uint256 indexed adId)
==>

event CreateEvent(uint256 indexed adId, uint256 indexed parentId,  uint256 price,  uint256 deposit, uint256 lastTaxPayTimestamp, string content)
event TaxPayEvent(uint256 indexed adId, uint256 indexed copyrightAmount, uint256 indexed administrationAmount);

event ChangePriceEvent(uint256 indexed adId)
==> event ChangePriceEvent(uint256 indexed adId, uint256 price)

event ChangeContentEvent(uint256 indexed adId)
==> event ChangeContentEvent(uint256 indexed adId, string content)

event AddDepositEvent(uint256 indexed adId)
==> event AddDepositEvent(uint256 indexed adId, uint256 depositToAdd)

event WithdrawDepositEvent(uint256 indexed adId)
==> event WithdrawDepositEvent(uint256 indexed adId, uint256 amount)

    /**
     * @dev Withdraw deposit of an Ad Board
     * @param adId id of the Ad Board.
     * @param amount amount to withdraw
     */
    function withdrawDeposit(uint256 adId, uint256 amount) public adBoardExists(adId) {
        require(msg.sender == ownerOf(adId));
        require(amount > 0);

        uint256 collected = 0;
        uint256 owed = 0;
        (collected, owed) = _collectTax(adId);

        AdBoardData storage data = allAdBoards[adId];
        _doCollectTax(adId, data.parentId, collected, 0);

        if (owed <= 0) {
            if (amount > data.deposit) {
                amount = data.deposit;
            }
            data.deposit = data.deposit.sub(amount);
            if (data.deposit <= 0) {
                data.price = 0;
            }
            if (amount > 0) {
                msg.sender.transfer(amount);
            }
        }

        emit WithdrawDepositEvent(adId);
    }

==>

    /**
     * @dev Withdraw deposit of an Ad Board
     * @param adId id of the Ad Board.
     * @param amount amount to withdraw
     */
    function withdrawDeposit(uint256 adId, uint256 amount) public adBoardExists(adId) {
        require(msg.sender == ownerOf(adId));
        require(amount > 0);

        uint256 collected = 0;
        uint256 owed = 0;
        (collected, owed) = _collectTax(adId);

        AdBoardData storage data = allAdBoards[adId];
        _doCollectTax(adId, data.parentId, collected, 0);

        if (owed <= 0) {
            if (amount > data.deposit) {
                amount = data.deposit;
            }
            data.deposit = data.deposit.sub(amount);
            if (data.deposit <= 0) {
                data.price = 0;
            }
            if (amount > 0) {
                msg.sender.transfer(amount);
                emit WithdrawDepositEvent(adId);
            }
        }
    }