Smart Contract Vulnerabilities in Societe Generale–FORGE’s CoinVertible Stablecoin
On April 20th, 2023, Societe Generale–FORGE (SG-FORGE) announced the release of CoinVertible, a euro-pegged stablecoin on the Ethereum public blockchain. Although the smart contract was audited, we noticed a few vulnerabilities that went undetected.
Lack of two-step role transfer (medium risk)
The Whitelist contract lacks a two-step ownership transfer of the registrar. This is in the updateRegistrar(address _newRegistrar) function. Although it performs a basic validation to check whether an address is zero, the address that receives the owner role isn’t covered correctly and is inaccessible.
Typos occur both intentionally and unintentionally when transferring ownership of the registrar. Whenever this is the case, it will brick the SmartCoin contract.
For CoinVertible to work as intended, ownership transfer is critical and demands the implementation of a two-step role transfer. As such, it’s best to implement a two-step role transfer to set the role recipient, enabling them to claim that role and finalize the role transfer.
Incorrect account for transfer approvals (high risk)
EURCV is highly permissioned by design and demands all transfers and transfer approvals to be validated by a central administrator, namely, the “registrar.” Unfortunately, in this smart contract, approvals are incorrectly computed in the rejectTransfer() function.
This opens the door to a whole host of issues. For example, if the registrar rejects a transaction request origination from a transformFrom() call, it could involuntarily provide an allowance to another user corresponding with the address. The smart contract could also get bricked because the integer overflow could make rejectTransfer(transferHash) always revert.
It’s best to replace transferRequest.to with transferRequest.spender (i.e., lines 80 and 85 of the smart contract).
When trying to recall funds from an account, the registrar could fall victim to a DDoS attack (low risk)
Whenever the registrar wants to recover funds from AccountA by calling the recall(A) function, the registrar will have the opportunity to front-run with a corresponding account (if said account is still whitelisted) or another account (AccountB) with sufficient approval to spend funds stored in AccountA.
Either AccountA or AccountB can call transfer() or transferFrom() whenever they see the registrar recall the transaction in Mempool. It will immediately increase _engagedAmount[A] and decrease it by the same amount _availableBalance(A) whenever this happens. This will make the registrar transaction recover funds to revert.
This bug can be found in line 257, availableBalance(_from) >= _amount. If AccountA approves different accounts, this can be repeated continuously to DDoS the registrar. However, the registrar could always removeAddressFromWhitelist() to revoke access for all whitelisted accounts or those accounts that AccountA previously approved. Unfortunately, this approach could lead to blacklisting and high costs related to gwei.
The better option is to create a function reject_and_recall(bytes32[] memory list_of_hashes,address _from, uint256 _amount). This approach will reject all pending transactions that involve the _from address before recalling its funds. Or replace _availableBalance(_from) with super.balanceOf(_addr) in line 257.
Unintentional null allowance and race condition (medium risk)
Issues can arise when a whitelisted UserA requests approval for UserB by calling approve(AddressB,value). When this occurs, the system will reset UserB’s allowance to 0, pending the registrar’s decision to accept or reject the approval.
A couple of issues can arise from this smart contract design. For example, if the registrar rejects the approval through rejectApprove(_approveHash), UserB’s allowance will be null rather than reverting to its previous balance.
If UserA plans to increase approval, UserB must wait for the registrar to accept it before executing the transferFrom(UserA…) function again. In this scenario, using the ofapprove() function is unwise. Implementing the same logic in the increaseAllowance() and decreaseAllowance() functions is best to mitigate the front-running issue.
It’s also best to add to the inherited ERC20 contract two internal (instead of external) functions, _increaseAllowance()and _decreaseAllowance(), which enforce the same logic as the (originally external) ones from the OpenZeppelin library.
Remove line 113, in the approve() function of the smart contract:
if (currentAllowedAmount > 0) super._approve(_msgSender(), _to, 0)Replace with:
if (currentAllowedAmount >= _value) {
super._decreaseAllowance(_msgSender(), _to, currentAllowedAmount-_value);
return true;}In this case, _decreaseAllowance() won’t require a registrar’s approval. However, increaseAllowance() still requires the registrar. So, it’s important to replace in approve() method:
_approves[approveHash] = ApproveRequest(
_msgSender(),
_to,
_value,
ApproveStatus.Created
);Replace with:
_approves[approveHash] = ApproveRequest(
_msgSender(),
_to,
_value-currentAllowedAmount,
ApproveStatus.Created
);Replace with:
function _safeApprove(
address _from,
address _to,
uint256 _value
) internal onlyWhitelisted(_from) onlyWhitelisted(_to) {
super._approve(_from, _to, _value);
}Replace with:
function _safeApprove(
address _from,
address _to,
uint256 _value
) internal onlyWhitelisted(_from) onlyWhitelisted(_to) {
super._increaseAllowance(_from, _to, _value); missing Transfer event during a transfer (mandatory according to ERC20 standard),
}Other low-risk issues include typos, the futility of some unchecked blocks, missing a transfer event during a transfer (mandatory according to the ERC20 standard), and high gewi expenses.
Misspelled OpenZeppelin
Redundant balanceOf
Key takeaway
Always conduct multiple smart contract security audits with multiple established smart contract auditors. It’s also critical to conduct comprehensive smart contract audits whenever changes are made.
