Nested Finance contest - 0xngndev's results

Handle

0xngndev

Vulnerability details

Impact

In the NestedBuybacker contract, the function claimFees is set to public, meaning anyone can call that function. The function makes a call to feeSplitter.releaseToken(), a function that calculates the amount of fees owed to the msg.sender, and transfers that amount to the msg.sender. If user A is a shareholder who is owed fees and calls the claimFees function, he would be the msg.sender of claimFees, but the NestedBuybacker contract would be the msg.sender of the internal call claimFees does to feeSplitter.releaseToken, meaning that feeSplitter.releaseToken would transfer tokens to NestedBuybacker instead of transferring them to User A.

Furthermore, there's no dust collector or withdraw function in the Nestedbuybacker contract, meaning that if this scenario ever plays out, those funds would be locked.

Recommended Mitigation Steps

An easy solution would be to make claimFees and internal function in NestedBuybacker. If an user wants to withdraw their fees, they can always call releaseToken in the feeSplitter contract, which is also public.

Handle

0xngndev

Vulnerability details

Impact

In FeeSplitter.sol by doing a small refactory gas can be saved in case of a revert in the functions: getAmountDue and _releaseToken . We can swap the order of two lines so we return earlier in case of a bad input, this way we save some gas because the evm would execute less opcodes before reverting.

Mitigation steps

getAmountDue: Swap line 83 with 84 to avoid computing unnecessary logic. Remove the "else" and combine it with line 83. Something like this:

  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));
        uint256 amountDue = (totalReceived * _tokenRecords.shares[_account]) /
            _tokenRecords.totalShares -
            _tokenRecords.released[_account];
        return amountDue;
    }

_releaseToken: move line 252 after the require in line 254. Like this:

  function _releaseToken(address _account, IERC20 _token) private returns (uint256) {
        uint256 amountToRelease = getAmountDue(_account, _token);
        require(amountToRelease != 0, "FeeSplitter: NO_PAYMENT_DUE");
        TokenRecords storage _tokenRecords = tokenRecords[address(_token)];

        _tokenRecords.released[_account] = _tokenRecords.released[_account] + amountToRelease;
        _tokenRecords.totalReleased = _tokenRecords.totalReleased + amountToRelease;

        return amountToRelease;
    }

Handle

0xngndev

Vulnerability details

Impact

In the contract NestedBuybacker, the internal function trigger calculates how much NST to send to the reserve by doing: uint256 toSendToReserve = balance - toBurn; and then makes a safeTransfer for that amount to the nstReserve contract: NST.safeTransfer(nstReserve, toSendToReserve); Because there's a possibility of the burn rate set to 100%, there can exist situations where the contract ends up transferring 0 NST to the reserve. This would incur in unnecessary gas costs that can be avoided with a simple if check. The if check increases the contract size from 6189 to 6195 bytes, and adds a small amount of gas cost. However, if the possibility of having the burn rate set to 100% is something feasible, avoiding that call should end up being cheaper than adding an if check.

Mitigation steps

Simply add an if check to the NST.safeTransfer line that checks that toSendToReserve is not 0

Handle

0xngndev

Vulnerability details

Impact

Running a quick contract size check in the NestedFactory contract, I noticed it sat at 27590 bytes, exceeding the allowed 24576 bytes to deploy on mainnet. I removed the require messages alone in that contract and found the contract size dropped to 23172 bytes. Considering you are using large require messages in all the codebase, I would suggest considering a change of approach as to how you expose the error messages. I'll add my suggestions below.

Recommended Mitigation Steps

Two ways:

1. Shorten the length of the string messages to just the error instead of including the contract and the function. UniswapV3 repo may be a good example of how to do this. You can always explain errors further in the natspec, or in your documentation (you can make a common errors section).
2. Change require statements for if (...) revert CustomError(). Per solidity docs:

"Using a custom error instance will usually be much cheaper than a string description, because you can use the name of the error to describe it, which is encoded in only four bytes. A longer description can be supplied via NatSpec which does not incur any costs."

Link: https://docs.soliditylang.org/en/v0.8.10/control-structures.html?highlight=error#revert

Tools Used

dapptools make size