Platform: Code4rena
Start Date: 18/11/2021
Pot Size: $50,000 USDC
Total HM: 18
Participants: 26
Period: 7 days
Judge: leastwood
Total Solo HM: 12
Id: 54
League: ETH
Rank: 3/26
Findings: 3
Award: $5,371.72
🌟 Selected for report: 5
🚀 Solo Findings: 1
🌟 Selected for report: elprofesor
3984.587 USDC - $3,984.59
elprofesor
UnlockProtocol attempts to calculate gas reimbursement using tx.gasprice, typically users who falsify tx.gasprice would lose gas to miners and therefore not obtain any advantage over the protocol itself. This does present capabilities for miners to extract value, as they can submit their own transactions, or cooperate with a malicious user, reimbursing a portion (or all) or the tx.gasprice used. As the following calculation is made;
uint tokensToDistribute = (estimatedGasForPurchase * tx.gasprice) * (125 * 10 ** 18) / 100 / udtPrice;
we can see that arbitrary tx.gasprices can rapidly inflate the tokensToDistribute. Though capped at maxTokens, this value can be up to half the total supply of UDT, which could dramatically affect the value of UDT potentially leading to lucrative value extractions outside of the pool.
Using an oracle service to determine the average gas price and ensuring it is within some normal bounds that has not been subjected to arbitrary value manipulation.
#0 - julien51
2021-12-11T16:18:46Z
we can see that arbitrary tx.gasprices can rapidly inflate the tokensToDistribute. Though capped at maxTokens, this value can be up to half the total supply of UDT, which could dramatically affect the value of UDT potentially leading to lucrative value extractions outside of the pool.
As you noted it would be capped by the the actual increase of the GDP transaction.
However we could indeed use an oracle to determine the average gas price over a certain number of blocks to limit the risk even further.
#1 - 0xleastwood
2022-01-11T01:16:49Z
I think the warden has raised a valid issue of value extractions. Whether the value extracted is capped at a certain number of tokens, I don't think the issue is nullified as a result. Miners can realistically fill up blockspace by abusing this behaviour and then selling netted tokens on the open market. I'll consider marking this as medium, what do you think @julien51 ?
#2 - 0xleastwood
2022-01-11T03:59:35Z
I think maxTokens will be set to IMintableERC20(udt).totalSupply() / 2 upon the first call to recordKeyPurchase(). If I'm not mistaken, this could allow a malicious miner could effectively distribute half of the token supply in one tx.
#3 - 0xleastwood
2022-01-16T04:51:07Z
After further offline discussions with @julien51. We agree that this is an issue that needs to be addressed.
If we consider real-world values for IMintableERC20(udt).totalSupply() and IMintableERC20(udt).totalSupply() as 1_000_000e18 and 400e18 respectively. Then a miner could mint up to ~1247 UDT tokens valued at $USD 124,688 if they provide a single Ether as their purchase amount. Obviously this can be abused to generate a huge amount of profit for miners, so as this is a viable way to extract value from the protocol, I will be keeping this as high severity.
🌟 Selected for report: elprofesor
Also found by: kenzo
elprofesor
The unlock protocols base contract Unlock.sol uses setLocktemplate() to initialize the implementation contract for the PublicLock proxy. This function will initialize the relevant PublicLock contract which has been deployed separately. PublicLock.initialize() does not have any relevant access control and does not prevent arbitrary users from initialising. This means that a malicious user could front run the setLocktemplate() forcing the deployer of PublicLock's implementation to redeploy. The process can be repeated, which costs the malicious user less than it would the owner of the Unlock Protocol, potentially unnecessarily draining funds from the development team.
Lack of access control on initialize
Implement valid access control on the PublicLock contract to ensure only the relevant deployer can initialize().
#0 - 0xleastwood
2022-03-22T21:46:50Z
I agree with the warden, _publicLockAddress is deployed separately and hence initialize can be called before setLockTemplate is called.
🌟 Selected for report: elprofesor
Also found by: 0x0x0x, harleythedog, loop, pauliax
elprofesor
Unlock.addLockTemplate() allows for onlyOwner() to specify the new version number, which contradicts expected usage elsewhere and can lead to gaps where there are no versions. This can amplify risk of denial of service found in # Insufficient version validation causes denial of service for PublicLock during lock upgrades.
function addLockTemplate(address impl, uint16 version) public onlyOwner { _publicLockVersions[impl] = version; _publicLockImpls[version] = impl; // @audit - Medium: Version increments don't match what is expected on the user end if (publicLockLatestVersion < version) publicLockLatestVersion = version; emit UnlockTemplateAdded(impl, version); }
As we can see, version is decided on by the onlyOwner, however in Unlock.upgradeLock() we can see that the following check is made:
// check version IPublicLock lock = IPublicLock(lockAddress); uint16 currentVersion = lock.publicLockVersion(); require(version == currentVersion + 1, 'version error: only +1 increments are allowed');
Where version is an input parameter and current version is defined from the lock contract. This may lead to version mismatching, preventing the PublicLock from being upgraded unless there is a valid version in the implementation that increments by one. It is worth noting that issues surrounding incorrect versions lead to potentially setting the implementation address of the proxy PublicLock to the zero address, which is not a recoverable state and acts as complete censorship of the content creator for more see separate issue (title: Insufficient version validation causes denial of service for PublicLock during lock upgrades).
These two issues are separate as they are based on two different attack vectors. This one focuses on purposeful actions or mistakes by the Unlock Protocol gnosis and/or dao. The latter focuses on accidentally surpassing the max version, which occurs due to a separate failure and not due to malicious or mistaken gnosis/dao.
lock.publicLockVersion returns 1function addLockTemplate(address impl, uint16 version) but increments version straight to 3require(version == currentVersion + 1, 'version error: only +1 increments are allowed'); will revert and prevent upgrades from occuringTypical security recommendations advise in upgrading to the latest version, we would also advise the same process to be observed. If this isn't in line with the team, it would be best to check whether an implementation exists in Unlock, rather than relying on the implementation contract itself (which is an external call).
#0 - julien51
2021-12-11T16:23:05Z
the only address that can submit new versions is the "owner" of the Unlock contract (team multisig and soon DAO). We believe that any such transaction should be thoroughly verified before being submitted.
#1 - 0xleastwood
2022-01-16T08:35:18Z
This issue can be resolved by calling addLockTemplate() to fill in the missing gap. Additionally, the sponsor has mentioned that the account allowed to call this function is likely to vet what data they provide. Hence, this issue is extremely unlikely but worth handling. I'll downgrade this to low.
#2 - 0xleastwood
2022-01-16T09:40:03Z
Duplicate of #39
#3 - 0xleastwood
2022-01-16T22:44:27Z
I think I'll mark this as the primary issue
🌟 Selected for report: elprofesor
elprofesor
When lock managers attempt to use Unlock.upgradeLock() there is insufficient validation of version for extracting the correct _publicLockImpls[version]. Instead, if upgradeLock() is called, and the PublicLock is already at the latest version, the code below does not account for version being higher than any currently available version;
IPublicLock lock = IPublicLock(lockAddress); uint16 currentVersion = lock.publicLockVersion(); require( version == currentVersion + 1, 'version error: only +1 increments are allowed'); // make our upgrade address impl = _publicLockImpls[version]; TransparentUpgradeableProxy proxy = TransparentUpgradeableProxy(lockAddress); proxyAdmin.upgrade(proxy, impl); // @audit - HIGH: Potentially using a version that is not available in `_publicLockImps[]` could result in null implementation here due to insufficient validation of version number
If version > publicLockLatestVersion then a null address is returned for address impl = _publicLockImpls[version]; on Line 249. This sets the implementation address of the proxy to the null address, in this case the implementation contains the logic of the lock contract that the creator has deployed to maintain membership. If this occurs, it is impossible to subsequently upgradeLock due to an external call made to the proxy's implementation on L245 (which would be null at the time of the subsequent upgrade). This effectively renders the relevant public lock unusable, which directly impacts invested memberships in that PublicLock. This also breaks the assumption that Unlock Protocol cannot directly sensor the creators.
Unlock.upgradeLock() without validating whether the lock is up to dateTester recommends using a check to validate publicLockLatestVersion, and updating to this version rather than incrementing by 1.
#0 - julien51
2021-12-11T16:21:20Z
I am not sure why the Lock Manager would purposefully destroy their own lock... but we could indeed be a bit stricter by adding some validation in order to prevent them from doing so (or an attacker who has stolen their private keys).
#1 - 0xleastwood
2022-01-16T08:37:09Z
According to Unlock's threat model, it isn't expected that a lock manager would wish to attack its own members. However, if the keys in control of this account were compromised, this would be a valid attack vector. I'll mark this as low as it's worth handling nonetheless.
🌟 Selected for report: elprofesor
elprofesor
PublicLock has incomplete fallback function which allows for the receipt of ether, but does not track who sent what, which could lead to funds becoming lost.
Decide whether this contract should accept or not accept funds, after which appropriate implementation of fallback should be finalised. Based on contract details it seems this contract shouldn't be receiving eth and therefore should reimburse ether sent to this contract. If this is incorrect, the implementation should track who sent how much to ensure funds are returned to appropriate users.
#0 - julien51
2021-12-11T16:16:03Z
I am not sure I understand. The lock contract MUST be able to receive some funds (Eth) when key purchases are made.
#1 - 0xleastwood
2022-01-16T09:26:44Z
The contract can already accept funds through the receive() function. fallback() is only hit if msg.data is provided on-top of msg.value. But if msg.data is empty, which is typically expected when sending ETH, then receive() is called.
#2 - 0xleastwood
2022-01-16T09:27:56Z
However, the fact that the contract accepts funds which are not utilised may cause issues to users who accidentally send ETH to the contract. This should be properly handled, even if receive() is updated to revert upon receiving funds.
#3 - 0xleastwood
2022-01-16T09:28:02Z
Marking as low as this is more to do with state handling.