Swivel contest - pauliax's results

Handle

pauliax

Vulnerability details

Impact

Different declared and the actual order of parameters for the Mature event:

event Mature(address indexed underlying, uint256 indexed maturity, uint256 maturityRate, uint256 matured); emit Mature(u, m, block.timestamp, currentExchangeRate);

maturityRate and matured are in the wrong order.

Recommended Mitigation Steps

This may trick the outside consumers of this event so consider fixing this.

Handle

pauliax

Vulnerability details

Impact

Another potential issue with this setFee function is that an admin can grief by frontrunning users and setting arbitrary high fees. The protocol should enforce lower and upper bounds for this fenominator. While this is a very theoretical issue and depends on the honesty of the owner, a similar issue was reported in a previous contest and assigned a score of medium: https://github.com/code-423n4/2021-05-nftx-findings/issues/51

Recommended Mitigation Steps

Introduce reasonable boundaries for the denominator values.

Handle

pauliax

Vulnerability details

Impact

I know this contract is not in scope but you should consider replacing your own Sig library that has some ToDos left with a battle-tested ECDSA library for signature verifications: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol

Recommended Mitigation Steps

This is just a recommendation to use ECDSA from OZ, didn't find a direct issue with your current approach.

Handle

pauliax

Vulnerability details

Impact

The variable domain in contract Swivel is cached in the contract storage and will not change after being initialized. However, if a hard fork happens after the contract deployment, the domain would become invalid on one of the forked chains due to the block.chainid has changed. A similar issue was reported in a previous contest and was assigned a severity of low: https://github.com/code-423n4/2021-06-realitycards-findings/issues/166

Recommended Mitigation Steps

An elegant solution that you may consider applying is from Sushi Trident: https://github.com/sushiswap/trident/blob/concentrated/contracts/pool/concentrated/TridentNFT.sol#L47-L62

Handle

pauliax

Vulnerability details

Impact

function setFee should validate that t is a valid index (0-3) and d > 0, as denominator must not be zero, it may produce division by 0 runtime error otherwise.

Recommended Mitigation Steps

Consider adding proposed validations.

Handle

pauliax

Vulnerability details

Impact

There are places where the same storage variable is accessed multiple times in the same function, e.g. function p2pZcTokenExchange accesses markets[u][m].zcTokenAddr 2 times, function matureMarket accesses markets[u][m] 3 times, and so on.

Recommended Mitigation Steps

It would be more gas efficient to cache these variables and re-use them where necessary.

Handle

pauliax

Vulnerability details

Impact

Consider using a fixed-size array for fenominator as the contract only assumes 4 fees anyway: /// @dev holds the fee demoninators for [zcTokenInitiate, zcTokenExit, vaultInitiate, vaultExit] uint16[] public fenominator; Also, there is no gas benefit of using lower than uint256 size variables here, actually quite the opposite as EVM operates on 32-byte values so it needs a conversion.

Recommended Mitigation Steps

Proposed solution: uint256 constant private NUMBER_OF_FEES = 4; uint256[NUMBER_OF_FEES] public fenominator;

Handle

pauliax

Vulnerability details

Impact

It is unclear why there are many functions that always return a boolean value of true. While this may be your agreed practice that you try to follow, it also incurs more gas consumption as the caller needs to receive and check these returned values.

Recommended Mitigation Steps

If you want to optimize for gas, consider dropping return values for functions that actually do not need them.

Handle

pauliax

Vulnerability details

Impact

boolean flag 'matured' could be removed if you agree to accept maturityRate > 0 as a matured vault, basically replacing: require(!matured, 'already matured'); with: require(maturityRate == 0, 'already matured'); This would eliminate one storage variable and thus reduce gas usage. The risk is that exchangeRateCurrent can never be 0 as this would mean an immature state.

Recommended Mitigation Steps

Consider getting rid of 'matured' as per suggestion.

Handle

pauliax

Vulnerability details

Impact

function matureVault could accept maturityRate as a parameter as this value was already fetched in Marketplace and can be passed to Vault to avoid a duplicate external call.

Recommended Mitigation Steps

function matureVault(uint256 maturityRate) {...} require(VaultTracker(markets[u][m].vaultAddr).matureVault(currentExchangeRate), 'maturity not reached');

Handle

pauliax

Vulnerability details

Impact

I do not see a reason why 'mature' and 'maturityRate' need to be stored in separate mappings, why they can't be included in the Market struct for better maintainability and gas reduction?

Recommended Mitigation Steps

before: struct Market { address cTokenAddr; address zcTokenAddr; address vaultAddr; } mapping (address => mapping (uint256 => Market)) public markets; mapping (address => mapping (uint256 => bool)) public mature; mapping (address => mapping (uint256 => uint256)) public maturityRate;

after: struct Market { address cTokenAddr; address zcTokenAddr; address vaultAddr; bool mature; uint256 maturityRate; } mapping (address => mapping (uint256 => Market)) public markets;

Handle

pauliax

Vulnerability details

Impact

Having both modifiers 'onlyAdmin' and 'onlySwivel' is not only more expensive but also misleading as these modifiers basically do the same job of checking an address against msg.sender.

Recommended Mitigation Steps

Better have a generalized modifier, something like onlyAddress(address a), and re-use it with both admin and swivel: modifier onlyAddress(address a) { require(msg.sender == a, 'sender not authorized'); _; } onlyAddress(admin) onlyAddress(swivel)