In the rOUSG.sol
contract, pausing functionality is not initialized by calling __Pausable_init()
inside the initialize
function. Thus, the contract cannot be paused
.
https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/rOUSG.sol#L58
contract ROUSG is
...
@> PausableUpgradeable,
...
{
As can be seen, the initialize
function lacks initialization of the pausing mechanism:
https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/rOUSG.sol#L102-L110
function initialize(
address _kycRegistry,
uint256 requirementGroup,
address _ousg,
address guardian,
address _oracle
) public virtual initializer {
__rOUSG_init(_kycRegistry, requirementGroup, _ousg, guardian, _oracle); // @doesn't init pausable
}
Pausing mechanism should be setup like so:
function initialize(
address _kycRegistry,
uint256 requirementGroup,
address _ousg,
address guardian,
address _oracle
) public virtual initializer {
__rOUSG_init(_kycRegistry, requirementGroup, _ousg, guardian, _oracle);
+ __Pausable_init();
}
In the rOUSG.sol
contract, the burn()
function emits the Transfer
event topics in the wrong order.
https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/rOUSG.sol#L638
function burn(
address _account,
uint256 _amount
) external onlyRole(BURNER_ROLE) {
...
_burnShares(_account, ousgSharesAmount);
ousg.transfer(
msg.sender,
ousgSharesAmount / OUSG_TO_ROUSG_SHARES_MULTIPLIER
);
@> emit Transfer(address(0), msg.sender, getROUSGByShares(ousgSharesAmount)); // @audit-low wrong order of topics
emit TransferShares(_account, address(0), ousgSharesAmount);
}
As can be seen in the EIP20, the Transfer
event should follow the below order:
https://eips.ethereum.org/EIPS/eip-20
event Transfer(address indexed _from, address indexed _to, uint256 _value)
A note from the EIP 20 specification:
A token contract which creates new tokens SHOULD trigger a Transfer event with the _from address set to 0x0 when tokens are created.
But as we can see above in the Transfer
event implementation of the burn()
function, the topics are in the wrong order as we are not creating or minting new tokens to be setting the from
address as the 0x0
address. Instead, the from
address should be the address the rOUSG
is being burnt from and the to
address should be the 0x0
address.
Hence, the order of topics should be as follows:
function burn(
address _account,
uint256 _amount
) external onlyRole(BURNER_ROLE) {
...
_burnShares(_account, ousgSharesAmount);
ousg.transfer(
msg.sender,
ousgSharesAmount / OUSG_TO_ROUSG_SHARES_MULTIPLIER
);
+ emit Transfer(_account, address(0), getROUSGByShares(ousgSharesAmount));
- emit Transfer(address(0), msg.sender, getROUSGByShares(ousgSharesAmount));
emit TransferShares(_account, address(0), ousgSharesAmount);
}
In the rOUSG.sol
contract, roles are not uniformly named and hashed into their bytes32
equivalent.
https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/rOUSG.sol#L94
bytes32 public constant BURNER_ROLE = keccak256("BURN_ROLE"); <--
A better naming to follow similarly how the PAUSER_ROLE
was setup should be:
bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
- bytes32 public constant BURNER_ROLE = keccak256("BURN_ROLE");
+ bytes32 public constant BURNER_ROLE = keccak256("BURNER_ROLE");
[N-2] _mintShares
can do without the whenNotPaused
modifier as the wrap()
function already enforces it
In the rOUSG.sol
contract, minting of shares are possible when the contract is not paused. When two functions (public/external & private/internal) reference the same whenNotPaused
modifier, the inner function can do without the modifier in the case that such inner function won't be reached any other way other than the outer function.
https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/rOUSG.sol#L415
function wrap(uint256 _OUSGAmount) external whenNotPaused { // note: pause mechanism protection
...
@> _mintShares(msg.sender, ousgSharesAmount);
...
}
https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/ousg/rOUSG.sol#L532
function _mintShares(
address _recipient,
uint256 _sharesAmount
) internal whenNotPaused returns (uint256) { <-----@
...
}
Get rid the inner whenNotPaused
modifier in the _mintShares
function:
function _mintShares(
address _recipient,
uint256 _sharesAmount
+ ) internal returns (uint256) {
- ) internal whenNotPaused returns (uint256) {
...
}