Platform: Code4rena
Start Date: 20/10/2021
Pot Size: $30,000 ETH
Total HM: 5
Participants: 15
Period: 3 days
Judge: 0xean
Total Solo HM: 3
Id: 44
League: ETH
Rank: 6/15
Findings: 1
Award: $589.46
🌟 Selected for report: 2
🚀 Solo Findings: 0
leastwood
The constructor() and several other functions do not validate input data. As a result, users and/or privileged roles may mistakenly call the respective functions with improper inputs, leading to unintended consequences and incorrect contract states.
https://github.com/code-423n4/2021-10-tally/blob/main/contracts/swap/Swap.sol#L51-L58 https://github.com/code-423n4/2021-10-tally/blob/main/contracts/swap/Swap.sol#L73-L76 https://github.com/code-423n4/2021-10-tally/blob/main/contracts/swap/Swap.sol#L106-L114
Manual code review
Consider checking for the zero address if the input variable is of type address. Additionally, swapFee in the constructor() should be checked to ensure it is strictly less than SWAP_FEE_DIVISOR.
#0 - Shadowfiend
2021-10-29T21:07:49Z
Consider checking for the zero address if the input variable is of type address.
feeRecipient is intentionally 0-settable (see the NatSpec). Most other addresses in the linked sections are checked for 0-ness, except for zrxTo; will double-check if there's more validation we can do there.
Additionally, swapFee in the constructor() should be checked to ensure it is strictly less than SWAP_FEE_DIVISOR.
Great catch!
#1 - Shadowfiend
2021-11-03T20:58:02Z
Ultimately given the above, marking this a duplicate of #25.
🌟 Selected for report: leastwood
Also found by: elprofesor
220.3579 USDC - $220.36
leastwood
Swap.sol inherits OpenZeppelin's Ownable contract which enables the onlyOwner role to transfer ownership to another address. It's possible that the onlyOwner role mistakenly transfers ownership to the wrong address, resulting in a loss of the onlyOwner role.
https://github.com/code-423n4/2021-10-tally/blob/main/contracts/governance/EmergencyGovernable.sol https://github.com/code-423n4/2021-10-tally/blob/main/contracts/swap/Swap.sol
Manual code review.
Consider overriding the transferOwnership() function to first nominate an address as the pending owner and implementing an acceptOwnership() function which is called by the pending owner to confirm the transfer. Alternatively, as the onlyOwner role is not used throughout the contract, it may be useful to remove this contract entirely from the EmergencyGovernable.sol contract.
#0 - Shadowfiend
2021-10-29T21:10:23Z
Alternatively, as the onlyOwner role is not used throughout the contract, it may be useful to remove this contract entirely from the EmergencyGovernable.sol contract.
Great catch. I think this was a vestige from early iterations.
220.3579 USDC - $220.36
leastwood
The sweepFees() function utilises OpenZeppelin's SafeERC20 library to ensure token transfers are successful. In the event of an unsuccessful token transfer, the transaction will revert with a relevant revert message. However, due to the nature of these reverts, it's possible for one of these tokens to intentionally or unintentionally revert to deny execution of the initial sweepFees() call. This may result in considerable gas costs if the function has already iterated a number of times.
https://github.com/code-423n4/2021-10-tally/blob/main/contracts/swap/Swap.sol#L243-L259
Manual code review
Consider implementing Solidity's try and catch statement to correctly emit an event if a single token sweep was unsuccessful without denying execution of the entire call to sweepFees().
#0 - Shadowfiend
2021-10-29T21:03:18Z
Since sweepFees takes the token list to sweep as a parameter, the underlying problem can be mitigated with no additional code changes; in particular, the function can be called with fewer or different tokens if a token is found to error. A transaction call before a submission should eliminate most instances where this would be an issue.
That said, I'd still like to see if we can land the try/catch changes.
#1 - 0xean
2021-11-06T12:34:14Z
Going to downgrade the severity on this one as I don't see it an availability risk since the caller could modify the token list