Swivel contest - gpersoon's results

Handle

gpersoon

Vulnerability details

Impact

The functions exitZcTokenFillingZcTokenInitiate and exitVaultFillingVaultInitiate are kind of similar, however the processing of the fee is different. exitZcTokenFillingZcTokenInitiate send the fee from o.maker exitVaultFillingVaultInitiate sends the fee from msg.sender

It seem to me that exitVaultFillingVaultInitiate should also send the fee from o.maker. It's strange to first send uTokens (exlusing the fee) to msg.sender and then send "fee" tokens from msg.sender to address(this).

Proof of Concept

https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L241-L289

function exitZcTokenFillingZcTokenInitiate(Hash.Order calldata o, uint256 a, Sig.Components calldata c) internal { .... // transfer underlying from initiating party to exiting party, minus the price the exit party pays for the exit (premium), and the fee. uToken.transferFrom(o.maker, msg.sender, principalFilled - a - fee); // transfer fee in underlying to swivel uToken.transferFrom(o.maker, address(this), fee);

function exitVaultFillingVaultInitiate(Hash.Order calldata o, uint256 a, Sig.Components calldata c) internal { ... // transfer premium minus fee from maker to sender uToken.transferFrom(o.maker, msg.sender, premiumFilled - fee);

// transfer fee in underlying to swivel from sender
uToken.transferFrom(msg.sender, address(this), fee);

Tools Used

Recommended Mitigation Steps

Double check the fee send in exitVaultFillingVaultInitiate. It probably should be: uToken.transferFrom(o.maker, address(this), fee);

Handle

gpersoon

Vulnerability details

Impact

The function transferNotionalFrom of VaultTracker.sol uses temporary variables to store the balances. If the "from" and "to" address are the same then the balance of "from" is overwritten by the balance of "to". This means the balance of "from" and "to" are increased and no balances are decreased, effectively printing money.

Note: transferNotionalFrom can be called via transferVaultNotional by everyone.

Proof of Concept

https://github.com/Swivel-Finance/gost/blob/v2/test/vaulttracker/VaultTracker.sol#L144-L196

function transferNotionalFrom(address f, address t, uint256 a) external onlyAdmin(admin) returns (bool) { Vault memory from = vaults[f]; Vault memory to = vaults[t]; ... vaults[f] = from; ... vaults[t] = to; // if f==t then this will overwrite vaults[f]

https://github.com/Swivel-Finance/gost/blob/v2/test/marketplace/MarketPlace.sol#L234-L238 function transferVaultNotional(address u, uint256 m, address t, uint256 a) public returns (bool) { require(VaultTracker(markets[u][m].vaultAddr).transferNotionalFrom(msg.sender, t, a), 'vault transfer failed');

Tools Used

Recommended Mitigation Steps

Add something like the following: require (f != t,"Same");

Handle

gpersoon

Vulnerability details

Impact

The solidity function ecrecover is used, however the error result of 0 is not checked for. See documentation: https://docs.soliditylang.org/en/v0.8.9/units-and-global-variables.html?highlight=ecrecover#mathematical-and-cryptographic-functions "recover the address associated with the public key from elliptic curve signature or return zero on error. "

Now you can supply invalid input parameters to the Sig.recover function, which will then result 0. If you also set o.maker to be 0 then this will match and an invalid signature is not detected.

So you can do all kinds of illegal & unexpected transactions.

Proof of Concept

https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L476-L484 function validOrderHash(Hash.Order calldata o, Sig.Components calldata c) internal view returns (bytes32) { ... require(o.maker == Sig.recover(Hash.message(domain, hash), c), 'invalid signature'); return hash; }

https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Sig.sol#L16-L23 function recover(bytes32 h, Components calldata c) internal pure returns (address) { ... return ecrecover(h, c.v, c.r, c.s);

Tools Used

Recommended Mitigation Steps

Verify that the result from ecrecover isn't 0

Handle

gpersoon

Vulnerability details

Impact

The function createMarket of MarketPlace.sol doesn't check if the market already exists. So it could accidentally deploy a market (with has the same maturity timestamp) twice and overwrite the previous values of the market. The previously deployed market is no longer accessible and the funds are lost.

The risk is limited because the function is authorized via "onlyAdmin(admin)"

Proof of Concept

https://github.com/Swivel-Finance/gost/blob/v2/test/marketplace/MarketPlace.sol#L53-L70 function createMarket(address u,uint256 m,address c,string memory n,string memory s,uint8 d) public onlyAdmin(admin) returns (bool) { ... markets[u][m] = Market(c, zctAddr, vAddr);

Tools Used

Recommended Mitigation Steps

Add a check in the beginning of createMarket: require(markets[u][m].cTokenAddr != address(0),"Double");