Nibbl contest - minhquanym's results

1. Signature malleability of EVM's ecrecover in verify()

Impact

The permit function of NibblVault calls the Solidity ecrecover function directly to verify the given signatures. However, the ecrecover EVM opcode allows malleable (non-unique) signatures and thus is susceptible to replay attacks.

Although a replay attack seems not possible here since the nonce is increased each time, ensuring the signatures are not malleable is considered a best practice (and so is checking _signer != address(0), where address(0) means an invalid signature).

Proof of Concept

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L563-L564

SWC-117: Signature Malleability SWC-121: Missing Protection against Signature Replay Attacks

Recommended Mitigation Steps Use the recover function from OpenZeppelin's ECDSA library for signature verification.

2. Wrong revert message in initiateBuyout()

In initiateBuyout() function, it check if block.timestamp >= minBuyoutTime and revert with message minBuyoutTime < now. But the revert message should be

minBuyoutTime > now

Proof of concept

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L399

3. Typo

• Naming function consistently between initialize in Vault and initialise in Basket

1. Cache secondaryReserveBalance in _chargeFee() function can save gas

In NibbleVault._chargeFee() function, secondaryReserveBalance is loaded from storage 2 times and stored in storage 1 time. We can save gas by loading it to memory to do calculation and only save to storage after all. For example

uint256 _tmp = secondaryReserveBalance;
uint256 _maxSecondaryBalanceIncrease = fictitiousPrimaryReserveBalance - _tmp;

_tmp += _feeCurve;
secondaryReserveRatio = uint32((_tmp * SCALE * 1e18) / (initialTokenSupply * initialTokenPrice));

secondaryReserveBalance = _tmp;

Affected Code

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L222-L226

2. Should use _feeAdmin in check before transfer ETH to factory

In _chargeFee() function, it checks if _adminFeeAmt > 0 before sending ETH to factory. _adminFeeAmt is not sent amount but _feeAdmin and when _amount is small, _feeAdmin can be 0 even though _adminFeeAmt > 0 due to division round down. In that case, transfer 0 wei is unnecessary and cost gas. So we should check _feeAdmin > 0 instead.

Affected Codes

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L227

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L243

3. Should use constant for UPDATE_TIME to save gas

UPDATE_TIME is not changed anywhere in the codebase, so it should be marked constant to save gas

Affected Codes

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Utilities/NibblVaultFactoryData.sol#L6