Streaming Protocol contest - pauliax's results

Handle

pauliax

Vulnerability details

Impact

functions transfer and transferFrom could validate that 'to' is not an empty address to prevent accidental burns. recoverTokens could validate that recipient is not empty. Function creatorClaimSoldTokens and claimFees could prevent an empty destination. Similarly, acceptGov could check that pendingGov is not a zero address.

Recommended Mitigation Steps

Consider validating that mentioned address parameters are not empty.

Handle

pauliax

Vulnerability details

Impact

There are TODOs left in the code. While this does not cause any direct issue, it indicates a bad smell and uncertainty:

  // TODO: we could have start_time+stream_duration+depositlocktime as maturity-date
  // TODO: figure out sane salt, i.e. streamid + x? streamid guaranteed to be unique

Recommended Mitigation Steps

Consider either fixing TODOs or removing them to avoid confusion.

Handle

pauliax

Vulnerability details

Impact

function updateStreamInternal accepts a parameter 'who' but uses msg.sender when accessing the stream:

  function updateStreamInternal(address who) internal {
    ...
    TokenStream storage ts = tokensNotYetStreamed[msg.sender];

This will not update the expected stream when who != msg.sender. Currently, this function is only called from within the modifier, and the modifier is applied with msg.sender only, but still this risk needs to be addressed.

Recommended Mitigation Steps

Consider either updating 'who', or totally getting rid of this parameter to avoid confusion.

Handle

pauliax

Vulnerability details

Impact

function exit performs updateStream and then calls withdraw but withdraw also has a updateStream modifier so this update is executed twice. Because you probably need an exit to have an up to date ts.tokens, an option to consider is extracting an internal _withdraw function with no updateStream modifier and you can re-use it with both exit and withdraw.

Handle

pauliax

Vulnerability details

Impact

This can be simplified to avoid useless calculations:

  uint112 feeAmt = amount * 10 / 10000; // 10bps fee

  uint112 feeAmt = amount / 1000; // 0.1% fee

Handle

pauliax

Vulnerability details

Impact

Don't need an empty else clause here:

  if (!isSale) {
      _burn(msg.sender, amount);
  } else {
  }
  
  if (!isSale) {
    // not a straight sale, so give the user some receipt tokens
    _mint(msg.sender, trueDepositAmt);
  } else {
  }

Handle

pauliax

Vulnerability details

Impact

Assigned operations to constant variables are re-evaluated every time.

  bytes32 public constant PERMIT_TYPEHASH =
    keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)");

See https://github.com/ethereum/solidity/issues/9232

Change from 'constant' to 'immutable'.

Handle

pauliax

Vulnerability details

Impact

'unchecked' directive can be applied where overflow/underflow is not possible, e.g.:

  require(newBal <= type(uint112).max && newBal > prevBal, "erc");
  uint112 amt = uint112(newBal - prevBal);
  
  require(newBal <= type(uint112).max && newBal > prevBal, "erc");
  uint112 trueDepositAmt = uint112(newBal - prevBal);
  
  require(newBal < type(uint112).max && newBal > prevBal, "erc");
  amount = uint112(newBal - prevBal);

Handle

pauliax

Vulnerability details

Impact

I think variable 'amt' in function fundStream can be eliminated if you just re-use the 'amount'. This will save some gas.