Tally contest - TomFrenchBlockchain's results

Handle

TomFrench

Vulnerability details

Impact

Swap fee could potentially be set to 100% resulting in total loss of funds for users in the case of malicious/faulty governor contract.

Proof of Concept

Swap.sol allows setSwapFee to be called by the address listed as the timelock by its governor contract. It's clear that this is meant to prevent frontrunning of swaps by the governor. No governor/timelock contract is in scope but should these be malicious/faulty it may be possible for this timelock behaviour to be circumvented.

Recommended Mitigation Steps

Set a constant maximum value which you never expect the swap fee to exceed (e.g. a swap fee of 10% would discourage users from performing any swaps) This ensures that even in the case of the timelock failing then users are guaranteed to retain at least 90% of their tokens.

There's no real reason to allow a 100% fee so by capping the max fee to a more reasonable value concerns about the governance contracts can be mitigated.

Handle

TomFrench

Vulnerability details

Impact

Increased gas cost on swaps

Proof of Concept

The logic in the require statement at this link can be simplified: https://github.com/code-423n4/2021-10-tally/blob/c585c214edb58486e0564cb53d87e4831959c08b/contracts/swap/Swap.sol#L142-L152

(!signifiesETHOrZero(zrxBuyTokenAddress) && boughtERC20Amount >= minimumAmountReceived) ||(signifiesETHOrZero(zrxBuyTokenAddress) && boughtETHAmount >= minimumAmountReceived)

is equivalent to

signifiesETHOrZero(zrxBuyTokenAddress) ? boughtETHAmount >= minimumAmountReceived : boughtERC20Amount >= minimumAmountReceived

Recommended Mitigation Steps

Replace logic statement with simplified version.

Handle

TomFrench

Vulnerability details

Impact

Increased gas costs on sweepTokens

Proof of Concept

On L250, we use a uint8 as the for loop variable: https://github.com/code-423n4/2021-10-tally/blob/c585c214edb58486e0564cb53d87e4831959c08b/contracts/swap/Swap.sol#L250

Due to how the EVM natively works on 256 bit numbers, using a 8 bit number here introduces additional costs as the EVM has to properly enforce the limits of this smaller type.

See the warning at this link: https://docs.soliditylang.org/en/v0.8.0/internals/layout_in_storage.html#layout-of-state-variables-in-storage

Recommended Mitigation Steps

Change i to be a uint256.