Notional contest - leastwood's results

Handle

leastwood

Vulnerability details

Impact

Notional's governance framework utilises a fork of Compound's Governor Alpha and ERC20 token. These are denoted specifically as the GovernorAlpha.sol and NoteERC20.sol contracts. However, the GovernorAlpha.sol has a key difference when compared to Compound's implementation of the same contract. Notional's Governor Alpha contract inherits Openzeppelin's TimelockController.sol contract. This enables Notional's governance framework to enforce a timelock on any proposals, giving users time to exit before a potentially dangerous maintenance operation is applied. TimelockController.sol's executeBatch() function is vulnerable to reentrancy as the check to see if the previous call has been executed is done after an external call is made. This allows an executor to hijack the contract and make unexpected changes to the Notional smart contract system.

Proof of Concept

Bug outlined here. Fix is outline in this commit.

Tools Used

Sourced from publicly disclosed post by Immnuefi.

Recommended Mitigation Steps

Update Openzeppelin library to a version containing the commit fixing the bug (mentioned above). Tag v3.4.2-solc-0.7 in Openzeppelin's Github repository is an example of a compatible library that contains the aforementioned bug fix.

Handle

leastwood

Vulnerability details

Impact

The scripts/ folder outlines a number of deployment scripts used by the Notional team. Some of the contracts deployed utilise the ERC1967 upgradeable proxy standard. This standard involves first deploying an implementation contract and later a proxy contract which uses the implementation contract as its logic. When users make calls to the proxy contract, the proxy contract will delegate call to the underlying implementation contract. NoteERC20.sol and Router.sol both implement an initialize() function which aims to replace the role of the constructor() when deploying proxy contracts. It is important that these proxy contracts are deployed and initialized in the same transaction to avoid any malicious frontrunning. However, scripts/deployment.py does not follow this pattern when deploying NoteERC20.sol's proxy contract. As a result, a malicious attacker could monitor the Ethereum blockchain for bytecode that matches the NoteERC20 contract and frontrun the initialize() transaction to gain ownership of the contract. This can be repeated as a Denial Of Service (DOS) type of attack, effectively preventing Notional's contract deployment, leading to unrecoverable gas expenses.

Proof of Concept

https://github.com/code-423n4/2021-08-notional/blob/main/scripts/deployment.py#L44-L60 https://github.com/code-423n4/2021-08-notional/blob/main/scripts/mainnet/deploy_governance.py#L71-L105

Tools Used

Manual code review

Recommended Mitigation Steps

As the GovernanceAlpha.sol and NoteERC20.sol are co-dependent contracts in terms of deployment, it won't be possible to deploy the governance contract before deploying and initializing the token contract. Therefore, it would be worthwhile to ensure the NoteERC20.sol proxy contract is deployed and initialized in the same transaction, or ensure the initialize() function is callable only by the deployer of the NoteERC20.sol contract. This could be set in the proxy contracts constructor().

Handle

leastwood

Vulnerability details

Impact

There are a number of contracts which inherit UUPSUpgradeable.sol, namely; GovernanceAction.sol, PauseRouter.sol and NoteERC20.sol. All these contracts are deployed using a proxy pattern whereby the implementation contract is used by the proxy contract for all its logic. The proxy contract will make delegate calls to the implementation contract. This helps to facilitate future upgrades by pointing the proxy contract to a new and upgraded implementation contract. However, if the implementation contract is left uninitialized, it is possible for any user to gain ownership of the onlyOwner role in the implementation contract for NoteERC20.sol. Once the user has ownership they are able to perform an upgrade of the implementation contract's logic contract and delegate call into any arbitrary contract, allowing them to self-destruct the proxy's implementation contract. Consequently, this will prevent all NoteERC20.sol interactions until a new implementation contract is deployed.

Proof of Concept

Initial information about this issue was found here.

Consider the following scenario:

• Notional finance deploys their contracts using their deployment scripts. These deployment scripts leave the implementation contracts uninitialized. Specifically the contract in question is NoteERC20.sol.
• This allows any arbitrary user to call initialize() on the NoteERC20.sol implementation contract.
• Once a user has gained control over NoteERC20.sol's implementation contract, they can bypass the _authorizeUpgrade check used to restrict upgrades to the onlyOwner role.
• The malicious user then calls UUPSUpgradeable.upgradeToAndCall() shown here which in turn calls this function. The new implementation contract then points to their own contract containing a self-destruct call in its fallback function.
• As a result, the implementation contract will be self-destructed due the user controlled delegate call shown here, preventing all future calls to the NoteERC20.sol proxy contract until a new implementation contract has been deployed.

Tools Used

Manual code review

Recommended Mitigation Steps

Consider initializing the implementation contract for NoteERC20.sol and checking the correct permissions before deploying the proxy contract or performing any contract upgrades. This will help to ensure the implementation contract cannot be self-destructed.