Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: infinite approval doesn't persist after transfer #217

Merged
merged 3 commits into from
Jun 21, 2024
Merged

fix: infinite approval doesn't persist after transfer #217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c18fcc6
fix: infinite approval doesn't persist after transfer
danielattilasimon Jun 18, 2024
dbb111c
refactor: inherit OpenZeppelin's ERC20 in BoldToken
danielattilasimon Jun 19, 2024
b41b933
chore: update TODO
danielattilasimon Jun 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
214 changes: 23 additions & 191 deletions contracts/src/BoldToken.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,64 +2,29 @@

pragma solidity 0.8.18;

import "openzeppelin-contracts/contracts/token/ERC20/extensions/ERC20Permit.sol";
import "./Dependencies/Ownable.sol";

import "./Interfaces/IBoldToken.sol";
/*
*
* Based upon OpenZeppelin's ERC20 contract:
* https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol
*
* and their EIP2612 (ERC20Permit / ERC712) functionality:
* https://github.com/OpenZeppelin/openzeppelin-contracts/blob/53516bc555a454862470e7860a9b5254db4d00f5/contracts/token/ERC20/ERC20Permit.sol
*
*
* --- Functionality added specific to the BoldToken ---
*
* 1) Transfer protection: blacklist of addresses that are invalid recipients (i.e. core Liquity contracts) in external
* transfer() and transferFrom() calls. The purpose is to protect users from losing tokens by mistakenly sending Bold directly to a Liquity
* core contract, when they should rather call the right function.
*
* 2) sendToPool() and returnFromPool(): functions callable only Liquity core contracts, which move Bold tokens between Liquity <-> user.
*/

contract BoldToken is Ownable, IBoldToken {
uint256 private _totalSupply;
/*
* --- Functionality added specific to the BoldToken ---
*
* 1) Transfer protection: blacklist of addresses that are invalid recipients (i.e. core Liquity contracts) in external
* transfer() and transferFrom() calls. The purpose is to protect users from losing tokens by mistakenly sending Bold directly to a Liquity
* core contract, when they should rather call the right function.
*
* 2) sendToPool() and returnFromPool(): functions callable only Liquity core contracts, which move Bold tokens between Liquity <-> user.
*/

contract BoldToken is Ownable, IBoldToken, ERC20Permit {
string internal constant _NAME = "Bold Stablecoin";
string internal constant _SYMBOL = "Bold";
string internal constant _VERSION = "1";
uint8 internal constant _DECIMALS = 18;

// --- Data for EIP2612 ---

// keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)");
bytes32 private constant _PERMIT_TYPEHASH = 0x6e71edae12b1b97f4d1f60370fef10105fa2faae0126114a169c64845d6126c9;
// keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");
bytes32 private constant _TYPE_HASH = 0x8b73c3c69bb8fe3d512ecc4cf759cc79239f7b179b0ffacaa9a75d522b39400f;

// Cache the domain separator as an immutable value, but also store the chain id that it corresponds to, in order to
// invalidate the cached domain separator if the chain id changes.
bytes32 private immutable _CACHED_DOMAIN_SEPARATOR;
uint256 private immutable _CACHED_CHAIN_ID;

bytes32 private immutable _HASHED_NAME;
bytes32 private immutable _HASHED_VERSION;

mapping(address => uint256) private _nonces;

// TODO: is this needed?
uint256 public deploymentStartTime;

// User data for Bold token
mapping(address => uint256) private _balances;
mapping(address => mapping(address => uint256)) private _allowances;

// --- Addresses ---
/*
address public immutable troveManagerAddress;
address public immutable stabilityPoolAddress;
address public immutable borrowerOperationsAddress;
address public immutable activePoolAddress;
*/

// TODO: optimize to make them immutable
address public collateralRegistryAddress;
mapping(address => bool) troveManagerAddresses;
Expand All @@ -74,15 +39,7 @@ contract BoldToken is Ownable, IBoldToken {
event BorrowerOperationsAddressAdded(address _newBorrowerOperationsAddress);
event ActivePoolAddressAdded(address _newActivePoolAddress);

constructor() {
bytes32 hashedName = keccak256(bytes(_NAME));
bytes32 hashedVersion = keccak256(bytes(_VERSION));

_HASHED_NAME = hashedName;
_HASHED_VERSION = hashedVersion;
_CACHED_CHAIN_ID = _chainID();
_CACHED_DOMAIN_SEPARATOR = _buildDomainSeparator(_TYPE_HASH, hashedName, hashedVersion);

constructor() ERC20(_NAME, _SYMBOL) ERC20Permit(_NAME) {
deploymentStartTime = block.timestamp;
}

Expand Down Expand Up @@ -136,125 +93,18 @@ contract BoldToken is Ownable, IBoldToken {

// --- External functions ---

function totalSupply() external view override returns (uint256) {
return _totalSupply;
}

function balanceOf(address account) external view override returns (uint256) {
return _balances[account];
}

function transfer(address recipient, uint256 amount) external override returns (bool) {
function transfer(address recipient, uint256 amount) public override(ERC20, IERC20) returns (bool) {
_requireValidRecipient(recipient);
_transfer(msg.sender, recipient, amount);
return true;
return super.transfer(recipient, amount);
}

function allowance(address owner, address spender) external view override returns (uint256) {
return _allowances[owner][spender];
}

function approve(address spender, uint256 amount) external override returns (bool) {
_approve(msg.sender, spender, amount);
return true;
}

function transferFrom(address sender, address recipient, uint256 amount) external override returns (bool) {
_requireValidRecipient(recipient);
_transfer(sender, recipient, amount);
_approve(sender, msg.sender, _allowances[sender][msg.sender] - amount);
return true;
}

function increaseAllowance(address spender, uint256 addedValue) external override returns (bool) {
_approve(msg.sender, spender, _allowances[msg.sender][spender] + addedValue);
return true;
}

function decreaseAllowance(address spender, uint256 subtractedValue) external override returns (bool) {
_approve(msg.sender, spender, _allowances[msg.sender][spender] - subtractedValue);
return true;
}

// --- EIP 2612 Functionality ---
// TODO: remove and replace by openzeppelin implementation

function DOMAIN_SEPARATOR() public view override returns (bytes32) {
if (_chainID() == _CACHED_CHAIN_ID) {
return _CACHED_DOMAIN_SEPARATOR;
} else {
return _buildDomainSeparator(_TYPE_HASH, _HASHED_NAME, _HASHED_VERSION);
}
}

function permit(address owner, address spender, uint256 amount, uint256 deadline, uint8 v, bytes32 r, bytes32 s)
external
override
function transferFrom(address sender, address recipient, uint256 amount)
public
override(ERC20, IERC20)
returns (bool)
{
require(deadline >= block.timestamp, "Bold: expired deadline");
bytes32 digest = keccak256(
abi.encodePacked(
"\x19\x01",
DOMAIN_SEPARATOR(),
keccak256(abi.encode(_PERMIT_TYPEHASH, owner, spender, amount, _nonces[owner]++, deadline))
)
);
address recoveredAddress = ecrecover(digest, v, r, s);
require(recoveredAddress != address(0) && recoveredAddress == owner, "Bold: invalid signature");
_approve(owner, spender, amount);
}

function nonces(address owner) external view override returns (uint256) {
// FOR EIP 2612
return _nonces[owner];
}

// --- Internal operations ---

function _chainID() private view returns (uint256 chainID) {
assembly {
chainID := chainid()
}
}

function _buildDomainSeparator(bytes32 _typeHash, bytes32 _name, bytes32 _version) private view returns (bytes32) {
return keccak256(abi.encode(_typeHash, _name, _version, _chainID(), address(this)));
}

// --- Internal operations ---
// Warning: sanity checks (for sender and recipient) should have been done before calling these internal functions

function _transfer(address sender, address recipient, uint256 amount) internal {
assert(sender != address(0));
assert(recipient != address(0));

_balances[sender] = _balances[sender] - amount;
_balances[recipient] = _balances[recipient] + amount;
emit Transfer(sender, recipient, amount);
}

function _mint(address account, uint256 amount) internal {
assert(account != address(0));

_totalSupply = _totalSupply + amount;
_balances[account] = _balances[account] + amount;
emit Transfer(address(0), account, amount);
}

function _burn(address account, uint256 amount) internal {
assert(account != address(0));

_balances[account] = _balances[account] - amount;
_totalSupply = _totalSupply - amount;
emit Transfer(account, address(0), amount);
}

function _approve(address owner, address spender, uint256 amount) internal {
assert(owner != address(0));
assert(spender != address(0));

_allowances[owner][spender] = amount;
emit Approval(owner, spender, amount);
_requireValidRecipient(recipient);
return super.transferFrom(sender, recipient, amount);
}

// --- 'require' functions ---
Expand Down Expand Up @@ -291,22 +141,4 @@ contract BoldToken is Ownable, IBoldToken {
"Bold: Caller is neither TroveManager nor StabilityPool"
);
}

// --- Optional functions ---

function name() external pure override returns (string memory) {
return _NAME;
}

function symbol() external pure override returns (string memory) {
return _SYMBOL;
}

function decimals() external pure override returns (uint8) {
return _DECIMALS;
}

function version() external pure override returns (string memory) {
return _VERSION;
}
}
10 changes: 2 additions & 8 deletions contracts/src/Interfaces/IBoldToken.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@

pragma solidity 0.8.18;

import "openzeppelin-contracts/contracts/token/ERC20/IERC20.sol";
import "openzeppelin-contracts/contracts/token/ERC20/extensions/IERC20Metadata.sol";
import "openzeppelin-contracts/contracts/token/ERC20/extensions/IERC20Permit.sol";
import "openzeppelin-contracts/contracts/interfaces/IERC5267.sol";

interface IBoldToken is IERC20, IERC20Metadata, IERC20Permit {
interface IBoldToken is IERC20Metadata, IERC20Permit, IERC5267 {
function setBranchAddresses(
address _troveManagerAddress,
address _stabilityPoolAddress,
Expand All @@ -16,18 +16,12 @@ interface IBoldToken is IERC20, IERC20Metadata, IERC20Permit {

function setCollateralRegistry(address _collateralRegistryAddress) external;

function version() external pure returns (string memory);

function deploymentStartTime() external view returns (uint256);

function mint(address _account, uint256 _amount) external;

function burn(address _account, uint256 _amount) external;

function increaseAllowance(address spender, uint256 addedValue) external returns (bool);

function decreaseAllowance(address spender, uint256 subtractedValue) external returns (bool);

function sendToPool(address _sender, address poolAddress, uint256 _amount) external;

function returnFromPool(address poolAddress, address user, uint256 _amount) external;
Expand Down
29 changes: 29 additions & 0 deletions contracts/src/test/BoldToken.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.18;

import "./TestContracts/DevTestSetup.sol";

contract BoldTokenTest is DevTestSetup {
// TODO: need more tests for:
// - transfer protection
// - sendToPool() / returnFromPool()

function test_InfiniteApprovalPersistsAfterTransfer() external {
uint256 initialBalance_A = 10_000 ether;

openTroveHelper(A, 0, 100 ether, initialBalance_A, 0.01 ether);
assertEq(boldToken.balanceOf(A), initialBalance_A, "A's balance is wrong");

vm.prank(A);
assertTrue(boldToken.approve(B, UINT256_MAX));
assertEq(boldToken.allowance(A, B), UINT256_MAX, "Allowance should be infinite");

uint256 value = 1_000 ether;

vm.prank(B);
assertTrue(boldToken.transferFrom(A, C, value));
assertEq(boldToken.balanceOf(A), initialBalance_A - value, "A's balance should have decreased by value");
assertEq(boldToken.balanceOf(C), value, "C's balance should have increased by value");
assertEq(boldToken.allowance(A, B), UINT256_MAX, "Allowance should still be infinite");
}
}
Loading