Swivel contest - leastwood's results

Handle

leastwood

Vulnerability details

Impact

There are a number of functions which interact with uToken in Swivel.sol but do not check the return value of the associated transfer(), approve() or transferFrom() call. As a result, if a market is created using a non-standard token, a failed call will be reflected with a boolean return value instead of a revert. As a result, a failed transfer may be treated as a successful action.

Proof of Concept

https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L103 https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L104 https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L109 https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L137 https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L139 https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L144 https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L253 https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L255 https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L280 https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L283 https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L313 https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L315 https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L346 https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L347 https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L416 https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L419 https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L396

Tools Used

Manual code review

Recommended Mitigation Steps

Consider using Openzeppelin's SafeERC20 library to ensure all approve(), transfer() and transferFrom() are handled correctly.

Handle

leastwood

Vulnerability details

Impact

The onlyAdmin role points to an EOA account managed by the Swivel team. Withdrawals from the Swivel protocol have to be scheduled by the onlyAdmin role, ensuring that users have sufficient time to withdraw, if required.

The setFee() function in Swivel.sol does not have the same timelock, as seen in Swivel.withdraw(), enabling the onlyAdmin role to extract users' funds by intentionally updating the fenominator array in an instantaneous fashion. However, it is more likely that the admin role unintentionally makes an update while other transactions are in flight, resulting in unexpected fees for all users that are frontrun. This may also result in a number of reverted transactions due to insufficient uToken approvals.

Similarly, the setSwivelAddress() function in MarketPlace.sol can be used by the onlyAdmin role to drain the protocol by directly stealing funds or through ZcToken/nToken dilution. This is made possible via a sort of privilege escalation whereby the onlyAdmin role is able to take control of the onlySwivel role, giving access to any of the role's restricted functions.

Proof of Concept

https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L374-L397 https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L402-L405 https://github.com/Swivel-Finance/gost/blob/v2/test/marketplace/MarketPlace.sol#L42-L45

Consider the following attack vector:

• The onlyAdmin account calls setSwivelAddress() and updates the onlySwivel role to point to their account.
• The onlyAdmin account is able to call any function with the onlySwivel role, effectively giving them the ability to transfer vault holdings, mint and burn ZcTokens and add/remove nTokens.
• The following functions are susceptible; mintZcTokenAddingNotional, burnZcTokenRemovingNotional, redeemZcToken, redeemVaultInterest, custodialInitiate, custodialExit, p2pZcTokenExchange, p2pVaultExchange, and transferVaultNotionalFee.
• By being able to control ZcToken and nToken holdings, the admin account is able to claim the underlying collateral locked up on the Compound protocol.

Tools Used

Manual code review

Recommended Mitigation Steps

Consider adding a similar timelock to the setFee() and setSwivelAddress() functions as seen in Swivel.withdraw(). This should provide users with sufficient time to exit the protocol if for whatever reason the onlyAdmin role attempts to maliciously update the protocol or users want to avoid being frontrun.