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: 5/15
Findings: 6
Award: $4,970.93
🌟 Selected for report: 4
🚀 Solo Findings: 1
0xsanson
Not every ERC20 token follows OpenZeppelin's recommendation. It's possible (inside ERC20 standard) that a transferFrom doesn't revert upon failure but returns false.
The code doesn't check these return values. For example uToken.transferFrom(msg.sender, o.maker, a); in initiateVaultFillingZcTokenInitiate can be exploited by the msg.sender to initiate a trade without sending any underlying.
grep 'transfer' Swivel.sol
editor
Consider using OpenZeppelin's library with safe versions of transfer functions.
#0 - JTraversa
2021-10-24T21:35:31Z
How does one chose a specific issue out of all the dupes?
I had chosen the one that was the most verbose and accurate (of a few very verbose responses). Just curious how we've ended up with this one hah.
0.2678 ETH - $793.82
0xsanson
In MarketPlace.sol, admin can change the swivel address at any time.
A malicious admin can abuse this by setting swivel to his personal address. Then they can call some functions (like p2pZcTokenExchange and p2pVaultExchange) to steal zc-tokens and n-tokens from users. By redeeming these, they can steal all funds from the protocol.
https://github.com/Swivel-Finance/gost/blob/v2/test/marketplace/MarketPlace.sol#L42
editor
Consider adding a timelock on setSwivelAddress or directly on admin. Alternatively add a require(swivel == address(0)) so it can be called only the first time.
#0 - JTraversa
2021-10-08T05:42:12Z
I need to check whether this is a duplicate. Will remove once reviewing tomorrow.
I'd also lower this to low severity as one of a number of admin functionality restrictions.
#1 - 0xean
2021-10-16T22:39:18Z
Downgraded to Risk 2 along with its duplicates based on assets not at direct risk
0xsanson
Only an admin can create a market by calling MarketPlace.createMarket.
With the current implementation, it's possible to create another market with the same underlying u and maturity m. Doing so would rewrite markets[u][m] with a new ZcToken and VaultTracker. This makes the previous market inaccessible, effectively causing the loss of users' balances of n-tokens and zc-tokens.
https://github.com/Swivel-Finance/gost/blob/v2/test/marketplace/MarketPlace.sol#L53-L70
editor
In createMarket consider checking if the market is already present, for example adding a require(markets[u][m].cTokenAddr == address(0)). If multiple markets with the same u and m are needed, it's possible to add an identifier index to separate them, but the code needs more restructuring.
#0 - 0xean
2021-10-16T23:09:25Z
dupe of #97
🌟 Selected for report: 0xsanson
0.9917 ETH - $2,940.08
0xsanson
The current implementation doesn't work with fee-on-transfer underlying tokens. Considering that Compound can have these kind of tokens (ex. USDT can activate fees), this issue can affect the protocol.
The problem arise when transferring tokens, basically blocking all functions in Swivel.sol for that particular token, since the contract wrongly assumes balances values. This becomes particularly problematic in the following scenario: a market for USDT is running without problems, then they activate the fee: this effectively blocks users from redeeming the underlying.
grep 'transfer' Swivel.sol for a complete list of affected lines (basically every tranfer or transferFrom of underlying tokens). Also grep 'redeemUnderlying' Swivel.sol.
For example:
require(CErc20(mPlace.cTokenAddress(u, m)).redeemUnderlying(redeemed) == 0, 'compound redemption failed'); // transfer underlying back to msg.sender Erc20(u).transfer(msg.sender, redeemed);
This would fail (revert) since the contract would have received less than redeemed tokens.
editor
If the protocol wants to use all possible Compund's tokens, a way to handle these tokens must be implemented. A possible way to do it is to check the balance of the contract before and after every time a token is transferred to see the effective quantity. To help keeping the code clear, a function like Compund's doTransferIn can be implemented.
#0 - JTraversa
2021-10-08T05:38:37Z
Will review further. I dont believe that any tokens on compound currently have fees. Although it is news to me that USDT has toggle-able fee's woops.
That said, given we have admin control over added assets, I'd probably also lower this to a low-risk if accepted.
#1 - 0xean
2021-10-15T01:17:38Z
2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.
based on this "leaking value" I would say it qualifies as a med-severity.
#2 - JTraversa
2021-10-24T22:08:28Z
We can account for the transfers in with a similar balance before transferFrom, and balance after check, in order to prevent additional deposits after a fee has been turned on.
That said, Im not sure we can account for redeemUnderlying easily because should a fee be turned on, all funds would just be stuck in our contract if we added a similar check? (a != balance2-balance1)
If a fee is turned on for USDT markets, there would be lost fee value, so if adding a check wouldn't work, the most reasonable response is just to make sure the market is pausable.
0xsanson
In Swivel, admin can use function setFee to set fenominator to any positive quantity (or zero).
To boost users' trust in the protocol, it would be best to have a minimum fenominator (=> max fee).
In fact, in the worst scenario, a malicious admin can set fenominator = 1 when an user is starting a big trade (seen from the mempool), moving all user's tokens into fees that later he can withdraw.
https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L402
editor
Consider adding a require(d >= MIN_FENOMINATOR) with a reasonable MIN_FENOMINATOR. Also consider emitting an event when fenominator is changed, so it can be seen easily off-chain.
#0 - JTraversa
2021-10-10T05:53:16Z
Included as duplicate of setFee sanitization + event tickets.
🌟 Selected for report: 0xRajeev
Also found by: 0xsanson, nikitastupin, pauliax
0xsanson
ChainId can change after a chain hardfork and therefore shouldn't be considered constant, otherwise replay attacks become possible.
This is used to compute domain in Swivel (also DOMAIN_TYPEHASH in Hash), which are used to sign/validate orders.
https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Hash.sol#L31 https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L44
editor
Consider storing in Swivel an initial chainId, then in validOrderHash check if block.chainId is different: in this case domain and DOMAIN_TYPEHASH need to be recalculated before hashing the message.
🌟 Selected for report: 0xsanson
0.0941 ETH - $278.98
0xsanson
Marketplace.calculateReturn can be rewritten from:
function calculateReturn(address u, uint256 m, uint256 a) internal returns (uint256) { // calculate difference between the cToken exchange rate @ maturity and the current cToken exchange rate uint256 yield = ((CErc20(markets[u][m].cTokenAddr).exchangeRateCurrent() * 1e26) / maturityRate[u][m]) - 1e26; uint256 interest = (yield * a) / 1e26; // calculate the total amount of underlying principle to return return a + interest; }
to:
function calculateReturn(address u, uint256 m, uint256 a) internal returns (uint256) { uint256 rate = CErc20(markets[u][m].cTokenAddr).exchangeRateCurrent(); return a*rate/ maturityRate[u][m]; }
Less math operations means less approximations and less gas used.
https://github.com/Swivel-Finance/gost/blob/v2/test/marketplace/MarketPlace.sol#L160-L167
editor
#0 - JTraversa
2021-10-24T19:18:36Z
0.0171 ETH - $50.84
0xsanson
There are multiple instances of divisions performed before multiplications, whereas the opposite is generally suggested. To mitigate the precision loss, a factor like 1e18 is multiplied and then divided, but this solution is arbitrary and can be avoided.
For example:
uint256 principalFilled = (((a * 1e18) / o.premium) * o.principal) / 1e18;
can be rewritten like:
uint256 principalFilled = a * o.principal / o.premium;
Run grep '1e18' Swivel.sol for a complete list.
grep, editor
Suggested checking all instances and trying to simplify the math.
#0 - JTraversa
2021-10-24T19:18:18Z