Royalty owner can steal Stakeholders fees

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-nested/blob/main/contracts/FeeSplitter.sol#L134-L140

Vulnerability details

Impact

Under certain circumstances, Royalty owner can steal all fees funds which were meant for Stakeholders

Proof of Concept

1. Navigate to updateShareholder function in FeeSplitter.sol#L134-L140 and observe that there is no check to validate if newly updated weight!=0

2. This can have disaster impact as shown below

3. Assume owner has set one stakeholder X with the weight 1 and Royalty weight as 1

4. Owner uses the updateShareholder function and sets the stakeholder X weight to 0, which is success

5. This makes the final new totalWeights as 1 (Only Royalty weight)

6. Factory calls the sendFees function with amount 1000.

function sendFees(IERC20 _token, uint256 _amount) external nonReentrant {
        uint256 weights;
        unchecked {
            weights = totalWeights - royaltiesWeight;
        }

        uint256 balanceBeforeTransfer = _token.balanceOf(address(this));
        _token.safeTransferFrom(_msgSender(), address(this), _amount);

        _sendFees(_token, _token.balanceOf(address(this)) - balanceBeforeTransfer, weights);
    }

7. Since both totalWeights and royaltiesWeight is 1 so weight becomes 1-1 =0

8. Amount 1000 is transffered to this contract

9. _sendFees does nothing as stakeholder weight is 0 and also weights passed is 0

10. Finally contract has balance fees with no stakeholder able to claim

11. Now Factory owner calls sendFeesWithRoyalties with amount 100

function sendFeesWithRoyalties(
        address _royaltiesTarget,
        IERC20 _token,
        uint256 _amount
    ) external nonReentrant {
        require(_royaltiesTarget != address(0), "FS: INVALID_ROYALTIES_TARGET");

        uint256 balanceBeforeTransfer = _token.balanceOf(address(this));
        _token.safeTransferFrom(_msgSender(), address(this), _amount);
        uint256 amountReceived = _token.balanceOf(address(this)) - balanceBeforeTransfer;

        uint256 royaltiesAmount = _computeShareCount(amountReceived, royaltiesWeight, totalWeights);

        _sendFees(_token, amountReceived, totalWeights);
        _addShares(_royaltiesTarget, royaltiesAmount, address(_token));

        emit RoyaltiesReceived(_royaltiesTarget, address(_token), royaltiesAmount);
    }

12. Since totalWeights is equal to Royalty weight so _computeShareCount gives full shares of 100 to Royalty owner which also sets _tokenRecords.totalShares to 100 at _addShares function

13. Now Royalty owner has to simply call releaseToken which computes the amount using getAmountDue. Now getAmountDue considers full contract balance which currently is 1000+100=1100

function getAmountDue(address _account, IERC20 _token) public view returns (uint256) {
        TokenRecords storage _tokenRecords = tokenRecords[address(_token)];
        if (_tokenRecords.totalShares == 0) return 0;

        uint256 totalReceived = _tokenRecords.totalReleased + _token.balanceOf(address(this));
        return
            (totalReceived * _tokenRecords.shares[_account]) /
            _tokenRecords.totalShares -
            _tokenRecords.released[_account];
    }

14. This means the amount received will be 1100 as shown below

totalReceived=1000+100=1100
returns 1100*100/100-0=1100

15. As we can see here, Royalty owner was able to obtain 1100 amount when only 100 amount was meant for him and rest 1000 was only meant for stakeholder who might get added later by owner

Recommended Mitigation Steps

Add a new check in updateShareholder which does not allows the updated weight to be 0

function updateShareholder(uint256 _accountIndex, uint96 _weight) external onlyOwner {
        require(_weight!=0, "Incorrect weight assigned");
.....
    }

[maximebrugel]

Bad owner manipulation. But we must check that.

[harleythedogC4]

As stated in the contest, the contracts will be owned by a multisig behind a 7 day timelock, so there is no actual threat here. However, this finding does have some value since the 0 check does appear in setRoyaltiesWeight and _addShareholder so it makes sense to add it to updateShareholder for consistency. So I am changing this to low severity.

[harleythedogC4]

Addressed this in the QA report #9. Closing here.