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: 3/15
Findings: 5
Award: $10,766.37
🌟 Selected for report: 8
🚀 Solo Findings: 1
1.4875 ETH - $4,410.12
itsmeSTYJ
Taker is charged fees twice in exitVaultFillingVaultInitiate() . Maker is transferring less than premiumFilled to taker and then taker is expected to pay fees i.e. taker's net balance is premiumFilled - 2*fee
function exitVaultFillingVaultInitiate(Hash.Order calldata o, uint256 a, Sig.Components calldata c) internal { bytes32 hash = validOrderHash(o, c); require(a <= (o.principal - filled[hash]), 'taker amount > available volume'); filled[hash] += a; uint256 premiumFilled = (((a * 1e18) / o.principal) * o.premium) / 1e18; uint256 fee = ((premiumFilled * 1e18) / fenominator[3]) / 1e18; Erc20 uToken = Erc20(o.underlying); // transfer premium from maker to sender uToken.transferFrom(o.maker, msg.sender, premiumFilled); // transfer fee in underlying to swivel from sender uToken.transferFrom(msg.sender, address(this), fee); // transfer <a> vault.notional (nTokens) from sender to maker require(MarketPlace(marketPlace).p2pVaultExchange(o.underlying, o.maturity, msg.sender, o.maker, a), 'vault exchange failed'); emit Exit(o.key, hash, o.maker, o.vault, o.exit, msg.sender, a, premiumFilled); }
#0 - 0xean
2021-10-16T22:47:20Z
based on
3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).
This is being upgraded to a high risk. The duplicate of it was at that level by the submitting warden and considering that fees are being incorrectly taken from the taker and not the maker, the maker ends up with a higher balance than expected and the taker has no way to recoup these fees (assets are now lost).
#1 - JTraversa
2021-10-16T23:07:39Z
Is that how it is interpreted? I'd assume that high risk would imply a valid attack path that a user could use to drain deposited funds based on that description.
I wont fight this one obviously, just think theres a clear differentiation between this and the other high risk issue.
0.0976 ETH - $289.35
itsmeSTYJ
createMarket() is a privileged function that can only be called by an admin but that doesn't necessarily mean that it is not susceptible to mistakes. Furthermore, it is a function that is called somewhat often so following murphy's law - anything can go wrong, will go wrong. Smart contracts are one of those things that cannot go wrong because you cannot take it down so it's always better to err on the side of caution.
If a market already exists but somehow the admin accidentally created the same market again, all positions in that market will be lost forever because functions in VaultTracker.sol can only be called by admin i.e. MarketPlace.sol but there is no way to call the old market from MarketPlace.sol
Require that markets[u][m] is address(0) before setting the market.
#0 - 0xean
2021-10-16T23:13:32Z
dupe #97
🌟 Selected for report: itsmeSTYJ
0.9917 ETH - $2,940.08
itsmeSTYJ
In initiateZcTokenFillingZcTokenExit() , this comment // transfer underlying tokens - the premium paid + fee in underlying to swivel (from sender) is incorrect because you are actually transferring the underlying tokens - premium paid to the maker (from sender) AND you have to pay fee separately to swivel.
initiateZCTokenFillingZcTokenExit means I want to sell my nTokens so that means a is the amount of principal I want to fill. Let's use a hypothetical example where I (taker) wants to fill 10 units of ZcTokenExit for maker.
So effectively, I (taker) should be paying a-premium to maker and fee to swivel.
function initiateZcTokenFillingZcTokenExit(Hash.Order calldata o, uint256 a, Sig.Components calldata c) internal { bytes32 hash = validOrderHash(o, c); require(a <= o.principal - filled[hash]), 'taker amount > available volume'); // Note: you don't need to wrap these in brackets because if you look at the https://docs.soliditylang.org/en/latest/cheatsheet.html#order-of-precedence-of-operators, subtraction will always go before comparison filled[hash] += a; uint256 premiumFilled = (((a * 1e18) / o.principal) * o.premium) / 1e18; uint256 fee = ((premiumFilled * 1e18) / fenominator[0]) / 1e18; // transfer underlying tokens - the premium paid in underlying to maker (from sender) Erc20(o.underlying).transferFrom(msg.sender, o.maker, a - premiumFilled); Erc20(o.underlying).transferFrom(msg.sender, swivel, fee); // transfer <a> zcTokens between users in marketplace require(MarketPlace(marketPlace).p2pZcTokenExchange(o.underlying, o.maturity, o.maker, msg.sender, a), 'zcToken exchange failed'); emit Initiate(o.key, hash, o.maker, o.vault, o.exit, msg.sender, a, premiumFilled); }
#0 - JTraversa
2021-10-07T16:56:15Z
Really good eye.
We had o.maker send the fee after receiving it (same # of transfers as suggested mitigation) and actually accidentally lost that in a larger organizational commit 😅 .
#1 - JTraversa
2021-10-24T21:47:33Z
🌟 Selected for report: itsmeSTYJ
itsmeSTYJ
Misleading comments
For these 4 functions, it should say "taker's init" instead of "taker's exit"
#0 - JTraversa
2021-11-03T19:41:08Z
🌟 Selected for report: itsmeSTYJ
itsmeSTYJ
There is no impact to the funds but to align with best practices, it is always better to update internal state before any external function calls.
For functions exitVaultFillingZcTokenExit() and exitZcTokenFillingVaultExit(), you should do mPlace.custodialExit(...) to update the internal accounting before transferring the tokens out.
itsmeSTYJ
Unnecessary math that doesn't help prevent rounding errors.
It would be better to convert fenominator into percentage (e.g. bps, 10000 = 100%) and use this percentage to calculate fee. You will still run the risk of rounding to 0 if the taker's position size is miniscule so in order to prevent this, you can require a minimum taker size so that when you multiple a with feePercent , you get a value larger than 10000.
#0 - 0xean
2021-10-16T23:28:59Z
changing to gas since this has no effect on code
🌟 Selected for report: itsmeSTYJ
0.0941 ETH - $278.98
itsmeSTYJ
Gas op. Since vlt.notional has to be updated in both branches of the if check, you can take vlt.notional out of both branches and skip the else check.
function addNotional(address o, uint256 a) public onlyAdmin(admin) returns (bool) { uint256 exchangeRate = CErc20(cTokenAddr).exchangeRateCurrent(); Vault memory vlt = vaults[o]; if (vlt.notional > 0) { uint256 yield; uint256 interest; // if market has matured, calculate marginal interest between the maturity rate and previous position exchange rate // otherwise, calculate marginal exchange rate between current and previous exchange rate. if (matured) { // Calculate marginal interest yield = ((maturityRate * 1e26) / vlt.exchangeRate) - 1e26; } else { yield = ((exchangeRate * 1e26) / vlt.exchangeRate) - 1e26; } interest = (yield * vlt.notional) / 1e26; // add interest and amount to position, reset cToken exchange rate vlt.redeemable += interest; } vlt.notional += a; vlt.exchangeRate = exchangeRate; vaults[o] = vlt; return true; }
#0 - JTraversa
2021-10-24T20:39:37Z
overlap/competing with #113 , ended up going with 113's recommendation which i think? should save slightly more gas.
🌟 Selected for report: itsmeSTYJ
0.0941 ETH - $278.98
itsmeSTYJ
You don't need to store maturity in VaultTracker.sol or ZcToken.sol because mapping (address => mapping (uint256 => bool)) public mature; should already cover it. This will help to remove unnecessary external calls and also reduce the number of maturity checks.
// In MarketPlace.sol function createMarket( address u, uint256 m, address c, string memory n, string memory s, uint8 d ) public onlyAdmin(admin) returns (bool) { require(swivel != address(0), 'swivel contract address not set'); // TODO can we live with the factory pattern here both bytecode size wise and CREATE opcode cost wise? address zctAddr = address(new ZcToken(u, n, s, d)); address vAddr = address(new VaultTracker(c, swivel)); markets[u][m] = Market(c, zctAddr, vAddr); emit Create(u, m, c, zctAddr, vAddr); return true; } ... function matureMarket(address u, uint256 m) public returns (bool) { require(block.timestamp >= m, "maturity not reached"); require(!mature[u][m], 'market already matured'); // set the base maturity cToken exchange rate at maturity to the current cToken exchange rate uint256 currentExchangeRate = CErc20(markets[u][m].cTokenAddr).exchangeRateCurrent(); maturityRate[u][m] = currentExchangeRate; // set the maturity state to true (for zcb market) mature[u][m] = true; // set vault "matured" to true require(VaultTracker(markets[u][m].vaultAddr).matureVault(), 'maturity not reached'); emit Mature(u, m, block.timestamp, currentExchangeRate); return true; }
// In VaultTracker.sol ... // uint256 public immutable maturity; // deleted this ... constructor(address c, address s) { admin = msg.sender; cTokenAddr = c; swivel = s; } ... function matureVault() external onlyAdmin(admin) returns (bool) { matured = true; maturityRate = CErc20(cTokenAddr).exchangeRateCurrent(); return true; }
// uint256 public immutable maturity; // deleted this ... constructor(address u, string memory n, string memory s, uint8 d) Erc2612(n, s, d) { admin = msg.sender; underlying = u; }
#0 - JTraversa
2021-10-07T16:37:15Z
Yea we'll think on this one. We added maturity to zcToken and VaultTracker so that you could query it from their contract addresses directly.
Thats really only useful for zcTokens though so we'll probably remove from VaultTracker and consider whether we think its appropriate for zcTokens.
#1 - JTraversa
2021-10-24T20:37:40Z
https://github.com/Swivel-Finance/gost/commit/6e473ff16ce60de8829f1153eb49297707e22166
Ended up not removing maturity from vaulttracker or zcToken to allow some marginal amount of UX around bookkeeping.
But we did remove a few redundant requires and a lot of the functionality within each related to their "maturity" given it overlapped with the requires within marketplace.
🌟 Selected for report: itsmeSTYJ
0.0941 ETH - $278.98
itsmeSTYJ
Since you are already querying the exchangeRate for the current block in MarketPlace.matureMarket() , might as well pass it along to VaultTracker.sol instead of querying it a second time.
// In VaultTracker.sol function matureVault(uint256 _maturityRate) external onlyAdmin(admin) returns (bool) { require(!matured, 'already matured'); require(block.timestamp >= maturity, 'maturity has not been reached'); matured = true; maturityRate = _maturityRate; return true; }
// In MarketPlace.sol function matureMarket(address u, uint256 m) public returns (bool) { require(!mature[u][m], 'market already matured'); require(block.timestamp >= ZcToken(markets[u][m].zcTokenAddr).maturity(), "maturity not reached"); // set the base maturity cToken exchange rate at maturity to the current cToken exchange rate uint256 currentExchangeRate = CErc20(markets[u][m].cTokenAddr).exchangeRateCurrent(); maturityRate[u][m] = currentExchangeRate; // set the maturity state to true (for zcb market) mature[u][m] = true; // set vault "matured" to true require(VaultTracker(markets[u][m].vaultAddr).matureVault(currentExchangeRate), 'maturity not reached'); emit Mature(u, m, block.timestamp, currentExchangeRate); return true; }
#0 - JTraversa
2021-10-24T20:26:42Z
🌟 Selected for report: itsmeSTYJ
0.0941 ETH - $278.98
itsmeSTYJ
Gas optimisation. In the function transferNotionalFee(), sVault.exchangeRate is only 0 for the very first time this function is called so the if check to see if sVault.exchangeRate != 0 is only used once to handle this edge case.
It makes more sense to set the exchangeRate when the vault is created and remove these if conditions.
constructor(uint256 m, address c, address s) { admin = msg.sender; maturity = m; cTokenAddr = c; swivel = s; uint256 exchangeRate = CErc20(cTokenAddr).exchangeRateCurrent(); vault[swivel] = Vault({ notional: 0, redeemable: 0, exchangeRate: exchangeRate }); } ... function transferNotionalFee(address f, uint256 a) external onlyAdmin(admin) returns(bool) { Vault memory oVault = vaults[f]; Vault memory sVault = vaults[swivel]; // remove notional from its owner oVault.notional -= a; uint256 exchangeRate = CErc20(cTokenAddr).exchangeRateCurrent(); uint256 yield; uint256 interest; // check if exchangeRate has been stored already this block. If not, calculate marginal interest + store exchangeRate if (sVault.exchangeRate != exchangeRate) { // if market has matured, calculate marginal interest between the maturity rate and previous position exchange rate // otherwise, calculate marginal exchange rate between current and previous exchange rate. if (matured) { // calculate marginal interest yield = ((maturityRate * 1e26) / sVault.exchangeRate) - 1e26; } else { yield = ((exchangeRate * 1e26) / sVault.exchangeRate) - 1e26; } interest = (yield * sVault.notional) / 1e26; sVault.redeemable += interest; sVault.exchangeRate = exchangeRate; } // add notional to swivel's vault sVault.notional += a; // store the adjusted vaults vaults[swivel] = sVault; vaults[f] = oVault; return true; }
#0 - JTraversa
2021-10-24T20:25:35Z