Platform: Code4rena
Start Date: 30/09/2021
Pot Size: $75,000 ETH
Total HM: 9
Participants: 15
Period: 7 days
Judge: 0xean
Total Solo HM: 2
Id: 39
League: ETH
Rank: 4/15
Findings: 7
Award: $10,021.01
🌟 Selected for report: 18
🚀 Solo Findings: 0
0xRajeev
For reference, see similar Medium-severity finding from Consensys Diligence Audit of Aave Protocol V2: https://consensys.net/diligence/audits/2020/09/aave-protocol-v2/#unhandled-return-values-of-transfer-and-transferfrom
As stated in the above finding: “ERC20 implementations are not always consistent. Some implementations of transfer and transferFrom could return ‘false’ on failure instead of reverting. It is safer to wrap such calls into require() statements to these failures.”
Swivel uses unchecked transfer/transferFrom in several places and given the protocol’s acceptance of arbitrary ERC20 tokens, this becomes very important.
Manual Analysis
Use require to check the return value and revert on 0/false or use OpenZeppelin’s SafeERC20 wrapper functions.
#0 - 0xean
2021-10-16T22:54:32Z
dupe of #155
🌟 Selected for report: gpersoon
Also found by: 0xRajeev, cmichel, nikitastupin
0xRajeev
Solidity’s ecrecover returns 0 if signature is invalid. The Sig.sol library does not perform zero address check on ecrecover’s return value and returns it as-is. The validOrderHash() function in Swivel which uses Sig.recover compares its return value directly with order maker again without zero-address checks. So if the signature is invalid and the maker address is accidentally zero, the order hash will be validated. This scenario will lead to burning of msg.sender’s tokens in certain functions.
This zero-address check is present in OZ lib: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/1b27c13096d6e4389d62e7b0766a1db53fbb3f1b/contracts/utils/cryptography/ECDSA.sol#L170-L173 from where Sig.sol is adapted. This indicates a danger of clone-and-own approach where it is generally safer to use widely used/reviewed libraries instead of copying/adapting them which could lose their security checks/fixes.
Reference: See similar Medium-severity finding M09 in OpenZeppelin's Audit of HoldeFi: https://blog.openzeppelin.com/holdefi-audit
Manual Analysis
Add zero-address check for ecrecover return value in Sig.sol or callers, or instead use OpenZeppelin’s ECDSA library.
#0 - 0xean
2021-10-16T22:53:59Z
dupe of #61
0.2678 ETH - $793.82
0xRajeev
Admin role has absolute power across Swivel, Marketplace and VaultTracker contracts with several onlyOwner functions. There is no ability to change admin to a new address or renounce it which is helpful for lost/compromised admin keys or to delegate control to a different governance/DAO address in future.
The project does not use the widely used OpenZeppelin Ownable library which provides transfer/renounce functions to mitigate such compromised/accidental situations with admin keys. This makes admin role/key a single-point of failure.
Manual Analysis
Ensure admins are reasonably redundant/independent (3/7 or 5/9) multisigs and add transfer/renounce functionality for admin. Consider using OpenZeppelin’s Ownable library.
#0 - JTraversa
2021-10-07T18:36:46Z
Similar to some other suggestions, i'm personally not sure if this is within the scope of the competition / an "issue". We have immediate plans to add admin transfer functionality however left it out of the scope of this audit as not having a transferAdmin function doesn't seem to be any more dangerous than the admin having the functionality it currently has.
That said, in production the admin will both be a multisig and there is also basic admin timelock/transfer functionality, with on-chain governance as opposed to centralized-ish multisigs coming Q1.
0xRajeev
onlyAdmin functions that change critical contract parameters/addresses/state should emit events and consider adding timelocks so that users and other privileged roles can detect upcoming changes (by offchain monitoring of events) and have the time to react to them.
Privileged functions in all contracts, for e.g. VaultTracker onlyAdmin, have direct financial or trust impact on users who should be given an opportunity to react to them by exiting/engaging without being surprised when changes initiated by such functions are made effective opaquely (without events) and/or immediately (without timelocks).
See similar Medium-severity finding in ConsenSys's Audit of 1inch Liquidity Protocol (https://consensys.net/diligence/audits/2020/12/1inch-liquidity-protocol/#unpredictable-behavior-for-users-due-to-admin-front-running-or-general-bad-timing)
and others
Manual Analysis
Add events to all possible flows (some flows emit events in callers) and consider adding timelocks to such onlyAdmin functions.
#0 - 0xean
2021-10-15T01:15:34Z
removing duplicate mark since the (valid) suggestion to emit events on the changes made by admin calls
0xRajeev
The createMarket function allows accidental overwriting of previously created markets for the same combination of underlying and maturity timestamp (u, m) because there is no zero-address check to see if a previously created market exists for that combination.
Manual Analysis
Add a zero-address check for markets[u][m] in createMarket before writing to it
#0 - JTraversa
2021-10-24T21:44:20Z
0.0325 ETH - $96.45
0xRajeev
Zero-address checks for input validation of address-type variables is a best-practice. While this is implemented in some places, there are missing ones.
Manual Analysis
Add zero-address checks.
#0 - JTraversa
2021-10-07T18:50:16Z
0x00 checks are reasonable to add. I've tended to err on the side of avoiding some input sanitization in scenarios where i'm sanitizing one or two specific inputs, when other input errors would be just as critical.
That said, I've always been confused by 0x00 address sanitization when any other address could be just as critical of an error. Does this not just add cost for very little potential benefit?
Would love to hear opinionated thoughts.
0xRajeev
The setFee onlyAdmin function sets the fee denominator but does not perform input validation to check that the fenominator index is between 0-3 which are the only valid values for [zcTokenInitiate, zcTokenExit, vaultInitiate, vaultExit].
The onlyAdmin function performs no threshold check on the new values, emits no event and immediately changes the fenominator value to any arbitrary value proposed by the admin.
Manual Analysis
Add input validation, threshold check, event and timelock
#0 - JTraversa
2021-11-03T17:00:59Z
0.1488 ETH - $441.01
0xRajeev
This implementation of Sig.sol doesn’t support compact signature (EIP-2098), where signature length can be 64 bytes instead of 65, as supported in the widely used OpenZeppelin’s ECDSA library. This lack of support could lead to DoS for users/clients that use compact signatures.
https://eips.ethereum.org/EIPS/eip-2098
Manual Analysis
Consider adding support for compact signatures, use OZ ECDSA library or highlight in documentation about this lack of support for EIP-2098.
#0 - JTraversa
2021-10-07T23:12:31Z
We'll consider just using the ECDSA library, looks like the library itself only added EIP-2098 in april? I honestly hadn't even heard of it.
0xRajeev
The initiate and exit functions that accept three arrays of orders, amounts and signatures fail to perform input validation to check if their lengths match. A mismatch could lead to exception in the best case or undefined behavior in the worst case.
Manual Analysis
Add input validation to check o.length == a.length == c.length.
🌟 Selected for report: 0xRajeev
0xRajeev
Interfaces are not allowed to define any functions while abstract contracts can have a few defined functions (with at least one undefined function). Abstract contracts declared in the project should really be interfaces because they do not define any functions.
Keeping them abstract is risky because they allow defining functions that may be mistakenly exposed in inherited contracts. Interfaces by design prevent this security risk.
Manual Analysis
Convert contracts that do not define any functions to interfaces.
#0 - JTraversa
2021-11-03T19:25:53Z
🌟 Selected for report: 0xRajeev
Also found by: 0xsanson, nikitastupin, pauliax
0xRajeev
The chainID captured at contract construction time is used for the domain in permit signature verification. As specified in the security considerations of EIP-2612, this has the following concern: "If the DOMAIN_SEPARATOR contains the chainId and is defined at contract deployment instead of reconstructed for every signature, there is a risk of possible replay attacks between chains in the event of a fututre chain split." See https://eips.ethereum.org/EIPS/eip-2612.
This is present both in ERC2612 implementation and order signature validation.
Manual Analysis
Create the domain dynamically for checks instead of statically at contract deployment time.
🌟 Selected for report: 0xRajeev
0xRajeev
Missing event and timelock for setSwivelAddress without which admin can change Swivel address at any time affecting the entire protocol.
Manual Analysis
Add event emit and a timelock to prevent opaque and immediate changes to the critical admin address. If this is never meant to be reinitialized, use a bool to prevent such a scenario.
#0 - JTraversa
2021-11-03T19:26:21Z
🌟 Selected for report: 0xRajeev
0xRajeev
blockWithdrawal does not perform input validation to check if input address is non-zero, indeed has a non-zero withdrawals time as scheduled earlier via scheduleWithdrawal or if it is being executed within/outside the HOLD period.
Without such validation, the block could be accidentally attempted for an incorrect address, performed without regard for HOLD period (the override should ideally be allowed only with the HOLD timelock period), and cannot be monitored off-chain given the absence of event emission. Given that this is an emergency function, lack of such input validation and event emission could critically delay/affect incident response.
The missing event for such a critical onlyAdmin override function (along with missing event for the withdaw() function) which unconditionally overrides previously scheduled onlyAdmin withdrawals is by itself a concern.
Manual Analysis
Add missing input validation on zero-address, non-zero value of withdrawals[e] and HOLD period consideration before resetting it to zero.
#0 - JTraversa
2021-10-10T06:29:43Z
Duplicate of an event ticket for admin functionality: setFee / blockWithdrawal
#1 - JTraversa
2021-11-03T19:18:02Z
0xRajeev
There are some public visibility calls across contracts that could be made external because they are not called from within contracts. Public visibility forces a copy of function parameters from calldata to memory which consumes gas depending on the number of parameters.
Manual Analysis
Evaluate all public visibility functions and set to external if they are not called from within contracts.
0.0423 ETH - $125.54
0xRajeev
There are few places across contracts where the same state variables are read multiple times or the same external calls are made multiple times within a function. Caching state variables or results from external calls in local/memory variables avoids SLOADs and CALLs to save gas. Warm SLOADs cost 100 gas and CALLs cost 2600 gas after Berlin upgrade. MLOADs cost only 3 gas units.
Cache swivel: https://github.com/Swivel-Finance/gost/blob/5fb7ad62f1f3a962c7bf5348560fe88de0618bae/test/marketplace/MarketPlace.sol#L61-L64
Cache markets[u][m]: https://github.com/Swivel-Finance/gost/blob/5fb7ad62f1f3a962c7bf5348560fe88de0618bae/test/marketplace/MarketPlace.sol#L76-L86
Cache matured and maturityRate: https://github.com/Swivel-Finance/gost/blob/5fb7ad62f1f3a962c7bf5348560fe88de0618bae/test/vaulttracker/VaultTracker.sol#L156-L179\
Cache vaults: https://github.com/Swivel-Finance/gost/blob/5fb7ad62f1f3a962c7bf5348560fe88de0618bae/test/vaulttracker/VaultTracker.sol#L244
Manual Analysis
Cache state variables or results from external calls in local/memory variables to save gas.
#0 - JTraversa
2021-10-08T05:09:35Z
I think this is the most comprehensive ticket for SLOAD optimizations/caching. Note for my own bookkeeping review.
#1 - JTraversa
2021-10-24T19:37:53Z
🌟 Selected for report: 0xRajeev
0.0941 ETH - $278.98
0xRajeev
The require(matureMarket(u, m) is redundant because matureMarket always returns true and reverts if any of its require() check fails.
Removing this can save a little gas.
Manual Analysis
Remove redundant require()
#0 - JTraversa
2021-10-24T19:26:33Z
🌟 Selected for report: 0xRajeev
0.0941 ETH - $278.98
0xRajeev
to.notional += a can be replaced by to.notional = a because to.notional = 0 in the else part. This will save a few MLOADs.
Manual Analysis
Replace to.notional += a by to.notional = a
#0 - JTraversa
2021-10-08T05:12:36Z
To review in comparison to just moving both notional += a blocks outside of the if
#1 - JTraversa
2021-10-24T19:23:57Z
0xRajeev
fenominator is declared as uint16[] but holds only 4 elements [zcTokenInitiate, zcTokenExit, vaultInitiate, vaultExit]. Changing it to uint16[3] will pack all four elements in one storage slot instead of using 2 slots in the case of a dynamic array where one slot is used for length.
Reading the static array will only perform one SLOAD instead of the two SLOADs required for the dynamic array.
Manual Analysis
Convert fenominator to a static array of uint16[3]
#0 - JTraversa
2021-10-24T19:21:05Z
🌟 Selected for report: 0xRajeev
0.0941 ETH - $278.98
0xRajeev
The local variable used as for loop index need not be initialized to 0 because the default value is 0. Avoiding this anti-pattern can save a few opcodes and therefore a tiny bit of gas.
Manual Analysis
Remove explicit 0 initialization of for loop index variable.
#0 - JTraversa
2021-10-24T19:20:51Z
🌟 Selected for report: 0xRajeev
0.0941 ETH - $278.98
0xRajeev
Swivel initiate() and exit() functions accept unbounded arrays from users which may lead to OOG exceptions with insufficient gas sent in transaction.
Manual Analysis
Bounding array lengths or checking gasleft are a good idea to reduce risk of OOG and save user’s gas.
#0 - JTraversa
2021-10-08T05:15:05Z
We intentionally left them unbounded, but on second thought could bound at the most we're even willing to serve from our BE's fillPreview endpoint at a time anywho.
edit: tagging for input sanitization
🌟 Selected for report: 0xRajeev
Also found by: csanuragjain
0.0423 ETH - $125.54
0xRajeev
For all functions taking user input of amount, requiring amount > 0 at the beginning of the function will save gas from avoiding execution of further logic for accidentally triggered zero amount transactions. This is a best-practice.
and some others.
Manual Analysis
Add require (amount > 0) at the beginning of the functions that accept any amount related variable as parameter.
#0 - JTraversa
2021-10-10T06:27:25Z
One of a few tickets on some input sanitization. If its best practice we'll be considering it, its minimal gas but the workflow leading to a 0 amount input would be difficult to do.