Tally contest - harleythedog's results

Handle

harleythedog

Vulnerability details

Impact

In the sweepFees function, address(this).balance is being used as the "amount" in the SweepFees event immediately after a transfer. So, the amount in the event on line 258 will always be 0, but it should be what address(this).balance was before the transfer. This has implications on overall functionality, tools that are monitoring this event will receive incorrect information. A fix is to store the value before calling the transfer.

Proof of Concept

referenced lines in sweepFees function: https://github.com/code-423n4/2021-10-tally/blob/main/contracts/swap/Swap.sol#:~:text=feeRecipient.transfer(address,this).balance%2C%20feeRecipient)%3B

A more correct implementation would be:

uint256 amount = address(this).balance; feeRecipient.transfer(address(this).balance); emit FeesSwept(address(0), amount, feeRecipient);

Tools Used

Manual inspection

Recommended Mitigation Steps

Store balance before calling transfer, as above.

Handle

harleythedog

Vulnerability details

Impact

In the sweepFees function, the storage variable feeRecipient is read multiple times and is read in the main loop as well. Each of these reads will be an expensive SLOAD operation. It would save a significant amount of gas if you were to store a memory copy of feeRecipient at the start of the function, and read this value instead (using the cheaper MLOAD operation instead).

Proof of Concept

sweepFees function here: https://github.com/code-423n4/2021-10-tally/blob/main/contracts/swap/Swap.sol#:~:text=function-,sweepFees,-(

A much more gas efficient function would look like this:

function sweepFees(
    address[] calldata tokens
) external nonReentrant {
    address payable recipient = feeRecipient; // read from this variable instead
    require(
        recipient != address(0),
        "Swap::withdrawAccruedFees: recipient is not initialized"
    );
    for (uint8 i = 0; i<tokens.length; i++) {
        uint256 balance = IERC20(tokens[i]).balanceOf(address(this));
        if (balance > 0) {
            IERC20(tokens[i]).safeTransfer(recipient , balance);
            emit FeesSwept(tokens[i], balance, recipient);
        }
    }
    recipient .transfer(address(this).balance);
    emit FeesSwept(address(0), address(this).balance, recipient);
}

Tools Used

Manual inspection.

Recommended Mitigation Steps

Change code to read from memory instead of storage as described above.

Handle

harleythedog

Vulnerability details

Impact

None of the events in swap.sol are indexed, so it is not easy for off-chain tools to efficiently filter these events. I would recommend adding indices to the SwappedTokens and FeesSwept events. For SwappedTokens, I would recommend adding an index on the tokenSold and tokenBought fields. For FeesSwept, I would recommend adding an index on the token and recipient fields.

Proof of Concept

See event declarations here: https://github.com/code-423n4/2021-10-tally/blob/main/contracts/swap/Swap.sol#:~:text=event-,SwappedTokens,-(

Tools Used

Manual inspection

Recommended Mitigation Steps

Added indices as described above.

Handle

harleythedog

Vulnerability details

Impact

In the setFeeRecipient and the constructor, the newFeeRecipient event is emitted. In both of these function, the value used is the feeRecipient storage variable. It would save a small amount of gas if you instead used the feeRecipient_ memory variable (less cost for MLOAD vs SLOAD) as this variable is equal to feeRecipient due to the previous line before the emit statement. This logic also applies to the NewSwapFee event emitted in the constructor.

Proof of Concept

setFeeRecipient here: https://github.com/code-423n4/2021-10-tally/blob/main/contracts/swap/Swap.sol#:~:text=function-,setFeeRecipient,-(address%20payable%20feeRecipient_

and constructor here: https://github.com/code-423n4/2021-10-tally/blob/main/contracts/swap/Swap.sol#:~:text=for%20each%20swap.-,constructor,-(address%20owner_%2C%20address

My suggestion is to change the lines:

emit NewFeeRecipient(feeRecipient);

to be:

emit NewFeeRecipient(feeRecipient_);

to save a little bit of gas. The same logic can be applied for the swapFee in the constructor.

Tools Used

Manual inspection.

Recommended Mitigation Steps

Change to use the memory variables as described above.