Unlock Protocol contest - TomFrenchBlockchain's results

Handle

TomFrench

Vulnerability details

Impact

Gas costs

Proof of Concept

Here we store the address of the proxy admin twice: https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/Unlock.sol#L98-L99

The motivation behind this seems to be to use the ProxyAdmin type internally but expose a getter which returns a plain address.

We could save gas by only storing this address once and having an explicit getter

function proxyAdminAddress() external view returns (address) {
  return address(proxyAdmin);
}

Internal usage of proxyAdminAddress for non-zero checks could be changed to compare against proxyAdmin instead as so proxyAdmin != ProxyAdmin(0)

Recommended Mitigation Steps

As above.

Handle

TomFrench

Vulnerability details

Impact

Gas costs

Proof of Concept

The version of lock contracts is stored and manipulated as a uint16 number.

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/Unlock.sol#L102-L103

https://github.com/code-423n4/2021-11-unlock/blob/ec41eada1dd116bcccc5603ce342257584bec783/smart-contracts/contracts/Unlock.sol#L245

There's no benefit to doing this as it doesn't take advantage of storage slot packing so we're just accepting greater costs of operating on non 32-byte words for no benefits.

Recommended Mitigation Steps

Change lock versions to be uint256 values.